PR#53: make critpath file optional - taskotron/taskotron-trigger

taskotron / taskotron-trigger

#53 make critpath file optional

Closed 6 years ago by kparal. Opened 6 years ago by kparal.

feature/critpath into develop

make critpath file optional

Kamil Páral • 6 years ago

94b1139

jobtriggers/koji_build_msg.py

file modified

+1 -2

		`@@ -23,9 +23,8 @@`
		`def _process_package(self, msg):`
		`name, version, release = self._get_nvr(msg)`
		`item = '-'.join([name, version, release])`
		`- critpath = utils.parse_yaml_from_file(config.critpath_filepath)`
		`distgit_branch = utils.get_distgit_branch(release)`
		`- critpath_pkgs = critpath['pkgs'].get(distgit_branch, critpath['pkgs']['master'])`
		`+ critpath_pkgs = utils.get_critpath_pkgs(release)`
		`data = {`
		`"_msg": msg,`
		`"message_type": "KojiBuildPackageCompleted",`

jobtriggers/utils.py

file modified

+12

		`@@ -150,3 +150,15 @@`
		`return False`

		`return True`
		`+`
		`+ def get_critpath_pkgs(release):`
		`+ '''Parse critpath definition file and return a list of critpath packages`
		`+ for a given release. If the file doesn't exist, return an empty list.'''`
		`+ if not os.path.exists(config.critpath_filepath):`
		`+ return []`
		`+ critpath = parse_yaml_from_file(config.critpath_filepath)`
		`+ if not critpath: # file is empty`
		`+ return []`
		`+ distgit_branch = get_distgit_branch(release)`
		`+ critpath_pkgs = critpath['pkgs'].get(distgit_branch, critpath['pkgs']['master'])`
		`+ return critpath_pkgs`

kparal commented 6 years ago

Don't crash if critpath file is missing or is empty. This is a
preparation for shutting down pkgdb where we pull critpath from
currently. Critpath is also available in PDC, but that's also going
away. New location is currently unknown. Since we don't use critpath in
any task at the moment, let's keep this functionality but don't fail
when the file is not present.

This has been tested shortly on taskotron-dev.

frantisekz commented 6 years ago

LGTM

kparal commented 6 years ago

Merged in e67e567

Pull-Request has been closed by kparal

6 years ago

jskladan commented 6 years ago

I know I=E2=80=99m on vacation, so code merges without review (sorry, Franti=
sku :)) are the new bread and butter, but please, revert this abomination.

This is heisenbug in the making, a proper (or at least acceptable) solution w=
oul be either adding a config option that enables the critpath file usage, o=
r (preferably) changing the code, so it allows for an empty (or null) =E2=80=
=9Ccritpath_filepath=E2=80=9D value, and then treats it as =E2=80=9Cno critp=
ath usage=E2=80=9D, while crashing on =E2=80=9Cwrong filepath=E2=80=9D.

The PR is error prone (imagine a typo in the config), and it also =E2=80=9Cf=
ails=E2=80=9D silently without any kind of log.

No :)=

kparal commented 6 years ago

Hi Josef, thanks for a vacation review :) I more or less agree with you, a config option would have been better. This has been already released and I need to deploy it to production asap, but I created a ticket (assigned to you:)) for a more reliable solution - #54.

Btw, I really tried to add logging in this patch. But the project doesn't have any custom logging set up, it seems that the only logger it uses is inherited from fedmsg or something like that, and I really wasn't sure how to handle that. And I needed the patch to be minimal, because we need to deploy it quickly. So that's why logging isn't there.

jskladan commented 6 years ago

Still not an OK thing to merge, solve this properly.=20

My temporary absence is not a reason to merge unacceptable code to my codeba=
se...=

kparal commented 6 years ago

See? A reason to not take a vacation, ever. People start committing unacceptable code. 🔱 🥢

An improved patch is in #56

Edited 6 years ago by kparal