#486 The compose scripts are a screaming ball of crazy
Opened 5 years ago by adamwill. Modified 5 years ago

Yeah, this is kinda silly, guys.

nightly.sh, nightly-modular.sh, twoweek-nightly.sh, cloud-nightly.sh and docker-nightly.sh are all basically the same script, with some bits left out and different values for the variables. This is silly, and wildly likely to lead to unintentional differences in behaviour (like, for e.g., the fact that the post-release scripts were not including the compose ID in fedmsgs, or that the post-release scripts were sending out 'complete' fedmsgs and doing rsyncs and stuff even for failed composes, because the check for pungi-koji's return code was missing from those versions).

Additionally, it seems like both Rawhide and Branched are built with nightly.sh, but Rawhide is built from the master branch version while Branched is built from the version on the fNN branch, where NN is the current Branched release number. This is pretty confusing and seems frankly unnecessary; we don't ever have more than one Branched, so why would we need release-specific branches of nightly.sh?

This all seems like it could be made a hell of a lot less redundant and confusing, perhaps along the lines of the old approach where all the real 'work' stuff was in a shared build-functions.sh and the build scripts just set variables to the correct values and then called functions from build-functions.sh. That seems like it was a much better approach.


To be clear on my intent here: I kinda backed into this issue via this post about broken compose reports, which led to me filing #484 and #485 . But in the course of doing that I dug into these scripts in enough detail that this really bugs me, and I'm actually intending to propose a rewrite of the scripts tomorrow (possibly to Python, because that way they can have actual tests and stuff). But I ran out of time to work on it today, so I figured I'd file a ticket to see if anyone has arguments in favour of the current approach, or other thoughts/plans, before I get too far into that.

You might be interested in this issue I opened a while back: https://pagure.io/releng/issue/6822

You might be interested in this issue I opened a while back: https://pagure.io/releng/issue/6822

I guess this request was more about the configs, though. You're point is that the scripts themselves are a ball of wax.

nightly.sh, nightly-modular.sh, twoweek-nightly.sh, cloud-nightly.sh and docker-nightly.sh are all basically the same script, with some bits left out and different values for the variables. This is silly, and wildly likely to lead to unintentional differences in behaviour

I agree to this and probably I will look into it making it more granular and just pass the necessary arguments to the functions.

we don't ever have more than one Branched, so why would we need release-specific branches of nightly.sh?

Its true, but branched is what becomes a release and we do the things the way the are because we need a way to track what we did for what releases. This can be also be done in one branch with git log or something, but in my view its way more complicated.

rewrite of the scripts tomorrow (possibly to Python, because that way they can have actual tests and stuff).

I dont think its necessary, my first comment(making the scripts granular) will solve this issue in my opinion.

@dustymabe

You might be interested in this issue I opened a while back: https://pagure.io/releng/issue/6822

We are not totally against the idea, but the plan to use ODCS for some of the releng compose tasks might cause some issues with include since ODCS doesn't support it yet.

"This can be also be done in one branch with git log or something, but in my view its way more complicated."

git tag f27-release

(or something like that), and you're done. Whereas having a branch does not achieve this on its own, as any mug could commit to the branch after the release is done. Like we're doing now, for the post-release work. The state of the f27 branch is already not what it was when we made Fedora-27-20171105.0, the Fedora 27 GA compose.

"I dont think its necessary, my first comment(making the scripts granular) will solve this issue in my opinion."

Honestly, I don't think tests are ever unnecessary for any critical process. But I'm not ordering anyone else to do this, just saying if I do go ahead and work on it, and no-one objects hard, that's how I'd probably do it.

A big +1 from me for tests & having the scripts written in Python (eq. something more sane than shell).

git tag f27-release

I am not against it, but when we put rawhide and branched in one git branch, when you tag a release it will also have rawhide configs/scripts.

as any mug could commit to the branch after the release is done.

Yes, its true and its possible, but people will not update the RC configs after they are released and if they did then there must be a mistake.

I am not against the idea, its just that there is no right way without any confusions.

I don't think tests are ever unnecessary for any critical process.

I agree tests are important, I just thought that they might not be necessary here. But I can talk to @ausil and see what we can do. Probably we need to make some system calls, other than that I dont see any bigger changes/issues with converting to python.

@mohanboddu I guess I'm mainly thinking about the nightly composes and not considering the release composes so much. I'll take a broader look before I rewrite anything. It's really the weirdness of having two entirely different nightly.shs, one on master for Rawhide and one on a release branch for Branched, that niggles at me.

Login to comment on this ticket.

Metadata