Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove buster, add failing bookworm jobs #373

Merged
merged 6 commits into from
Sep 2, 2022
Merged

Remove buster, add failing bookworm jobs #373

merged 6 commits into from
Sep 2, 2022

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Aug 25, 2022

Note, #372 must be merged before this one!

We'd like bookworm jobs to be able to fail, but when they do fail it
should make an appropriate level of noise so we can investigate why they
failed. But they should also not stop the bullseye packages from being
uploaded!

So we split the bookworm jobs into a separate track ("build2"), and have
separate push jobs for each version. The push-bullseye job goes before
the push-bookworm one in case bookworm jobs are failing.

Note that currently all the bookworm jobs are failing because of
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017046. I've put in a
fix for that, it should be in bookworm in the next 3-5 days.

Refs #369.

@legoktm
Copy link
Member Author

legoktm commented Aug 25, 2022

Here's what the nightly job ordering looks like:

Screenshot 2022-08-25 at 16-33-45 Workflow - securedrop-debian-packaging_1915_755b16f0-d3b9-46d9-b17b-9aff4cc588bb`

@eloquence
Copy link
Member

eloquence commented Aug 29, 2022

What should the policy on bookworm checks be? Should it be acceptable to merge PRs even if bookworm checks are failing?

If the answer is yes, just noting that, as I understand it, those commits will show up with a red "X" in the commit history on GitHub, regardless of whether the check is required or not. Not sure if there's a good way around that.

@legoktm
Copy link
Member Author

legoktm commented Aug 29, 2022

What should the policy on bookworm checks be? Should it be acceptable to merge PRs even if bookworm checks are failing?

Yes. I think people should quickly check to see if their change is causing the failure, and if so fix it (or less preferred, log an issue).

If the answer is yes, just noting that, as I understand it, those commits will show up with a red "X" in the commit history on GitHub, regardless of whether the check is required or not. Not sure if there's a good way around that.

Yeah :-(

@legoktm legoktm force-pushed the bookworm-or-bust branch 3 times, most recently from b9a4183 to 3a546dd Compare August 31, 2022 07:37
@legoktm
Copy link
Member Author

legoktm commented Aug 31, 2022

Hrmph.

# cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux bookworm/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

No $VERSION_CODENAME which we use in quite a few places... 🤔

This was already filed at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1008735

Because the bookworm release isn't finalized, it doesn't have the
$VERSION_ variables in /etc/os-release that we expect. We can parse
it out of $PRETTY_NAME, but it is complex enough that I wrote a tiny
`./scripts/codename` wrapper that contains the conditional and awk
~~magic~~ logic.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1008735 tracks adding
the $VERSION_ variables for testing, but it could easily end up as
"bookworm/sid", which still wouldn't work for us.
We'd like bookworm jobs to be able to fail, but when they do fail it
should make an appropriate level of noise so we can investigate why they
failed. But they should also not stop the bullseye packages from being
uploaded!

So we split the bookworm jobs into a separate track ("build2"), and have
separate push jobs for each version. The push-bullseye job goes before
the push-bookworm one in case bookworm jobs are failing.

Note that currently all the bookworm jobs are failing because of
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017046. I've put in a
fix for that, it should be in bookworm in the next 3-5 days.

Refs #369.
@legoktm
Copy link
Member Author

legoktm commented Aug 31, 2022

Remaining bookworm failures:

  • securedrop-client: needs some wheels to be built
  • securedrop-proxy: same
  • securedrop-workstation-grsec: I didn't copy over the changelog, leaving intentionally broken since it's going to go away soon I hope.

@legoktm
Copy link
Member Author

legoktm commented Aug 31, 2022

I'm going to leave removing the Python 3.7 wheels for another PR just so the resigning of the checksums file doesn't cause issues on rebase.

@eloquence
Copy link
Member

securedrop-client: needs some wheels to be built
securedrop-proxy: same

Do you want to do that as part of this PR before merging?

@legoktm
Copy link
Member Author

legoktm commented Aug 31, 2022

No strong preference on my end, I think if I get to it before this is reviewed/merged I can add it to this PR, otherwise it can go separately.

Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing how unfamiliar I am with this repo 😝 so I'll only comment.

  • The goal is clear to me ✅
  • The limitations in what we can do are too ✅
  • And with that in mind, the mechanics are clear too, with caveats that I think are acceptable (We'll be merging red builds somewhat regularly. Because it's a conscious decision I can live with that :P) ✔️
  • I'm not sure who signs the checksums file, so I haven't checked that. 🍊
  • I cannot spot any mistakes in the implementation. Take that with a grain of salt, but I also see that CI looks as expected, so I would go forward and ship it. ✔️

I'll let you give your thoughts too @eloquence !

@legoktm
Copy link
Member Author

legoktm commented Sep 1, 2022

  • I'm not sure who signs the checksums file, so I haven't checked that. 🍊

Anyone who has a key in the pubkeys/ folder can resign the file.

legoktm added a commit to freedomofpress/securedrop-proxy that referenced this pull request Sep 1, 2022
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how this change shows off #371's parameterization of this CI workflow!

In Monday's Workstation hangout we discussed #373 (comment) and agreed on a criterion for assessing provisional bookworm (and future Debian testing) builds (my summary notes):

If it's your change that's causing the bookworm job to fail, you should fix it before merge. If it's upstream, it's a judgment call.

As always, I would prefer to record this guidance to maintainers explicitly somewhere. @legoktm, do you think could be done usefully in a pull-request template here? or will that be necessary per upstream component repository (securedrop-client, etc.)?

@legoktm
Copy link
Member Author

legoktm commented Sep 1, 2022

As always, I would prefer to record this guidance to maintainers explicitly somewhere. @legoktm, do you think could be done usefully in a pull-request template here? or will that be necessary per upstream component repository (securedrop-client, etc.)?

Yes to both, in that bookworm job failures might need to be ignored here as well as in component repositories. I note that we currently don't even have a pull request template here :<, and the README is already a mess of documentation. Since it's a general policy that will span multiple repos, maybe it belongs better in the new developer docs?

@eloquence
Copy link
Member

My suggestion would be for a short paragraph into a PR template for now:

== Checklist ==

[ ] I've verified that this PR does not introduce any new bookworm test failures

Or similar.

@legoktm
Copy link
Member Author

legoktm commented Sep 2, 2022

OK, how's that?

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fbceaa8 nicely captures the spirit of "bookworm or bust". :-) Thanks, @eloquence and @legoktm! Approving and merging on the basis of #373 (review).

@cfm cfm merged commit 5b423ac into main Sep 2, 2022
@cfm cfm deleted the bookworm-or-bust branch September 2, 2022 00:28
@legoktm
Copy link
Member Author

legoktm commented Sep 2, 2022

Wheee, thank you to all 3 reviewers :-)

gonzalo-bulnes pushed a commit to freedomofpress/securedrop-client that referenced this pull request Sep 14, 2022
gonzalo-bulnes pushed a commit to freedomofpress/securedrop-client that referenced this pull request Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants