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

Removes Xenial related parts from codebase #5911

Merged
merged 12 commits into from
May 3, 2021
Merged

Removes Xenial related parts from codebase #5911

merged 12 commits into from
May 3, 2021

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Apr 21, 2021

Status

Ready for review.

Description of Changes

Fixes #5725

Removes major Xenial changes from the codebase.

Testing

  • CI should be green
  • visual inspection of the change

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you added or removed a file deployed with the application:

  • I have updated AppArmor rules to include the change

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

If you added or updated a code dependency:

Choose one of the following:

  • I have performed a diff review and pasted the contents to the packaging wiki
  • I would like someone else to do the diff review

@rmol
Copy link
Contributor

rmol commented Apr 21, 2021

I updated the Focal Dockerfile to get the layer cache updated.

@kushaldas
Copy link
Contributor Author

Should we remove/update the Debian packaging logic in all the packages for making them only Focal? Keeping them same will make the PR easier to review and lesser chances of breaking anything.

@emkll
Copy link
Contributor

emkll commented Apr 22, 2021

Should we remove/update the Debian packaging logic in all the packages for making them only Focal? Keeping them same will make the PR easier to review and lesser chances of breaking anything.

We should definitely remove the Xenial specific files and conditionals in the build logic where we test for Focal or Xenial, preserving only the Focal path. Our test coverage should detect any issue, provided we remove those as part of a separate commit. If that's too much in this PR, we can handle it in a separate PR.

@eloquence eloquence added this to the 2.0.0 milestone Apr 26, 2021
@conorsch
Copy link
Contributor

@kushaldas So far, so good! Heads up you'll need a rebase due to recent changes to CI config in #5907. Other than that, looks straightforward to press on and excise Xenial completely. 👍

kushaldas and others added 9 commits April 28, 2021 14:56
Ports the deb pkg tests into the focal directory, so the logic is
preserved.
We had a few dual-distro packages. The Xenial versions are now removed.
The postinst for the single "securedrop-app-code" pkg had some Xenial
logic, so that's been removed.
Conor Schaefer added 2 commits April 28, 2021 16:58
A few scripts have been updated, mostly it's the default for BASE_OS and
similar vars that now defaults to Focal.

The big change is snipping out all the special cases for Focal within
the CircleCI config, and using it by default everywhere.
@conorsch
Copy link
Contributor

@kushaldas Please take another look. I took the liberty of appending more removals, based on grepping around for "xenial" and similar strings. The results look pretty solid to me. There are a few exemptions that I've intentionally deferred:

  • molecule/{upgrade,vagrant-packager}/ we'll likely remove that entirely, but we've got it tracked in Support in-place upgrades for Focal VMs #5512
  • install_files/ansible-base/roles/app/tasks/configure_haveged.yml the longstanding Get rid of entropy checks #1584 is within reach again!
  • changelog entries (we've still got the Trusty changelog around, seems reasonable to keep the Xenial one too)

Other than that, we should be covered here. If you agree, please mark as ready-for-review, and I'll give it a final close look!

@kushaldas
Copy link
Contributor Author

Still can not figure out why are we seeing this particular CI failure due to the rebase-ci.sh file. The correct is present, screenshot below.

Screenshot from 2021-04-29 15-45-48

@kushaldas kushaldas marked this pull request as ready for review April 29, 2021 12:21
@kushaldas kushaldas requested a review from a team as a code owner April 29, 2021 12:21
@conorsch
Copy link
Contributor

Looks like CI is passing now—I'll take another look!

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Pretty complete here, the only missing changes I can point out are some of the EOL strings still present in the JI templates. See comment in-line. Once those changes are made, we'll need to update the strings queued up for translation, to remove the associated strings from the .po. I tried that locally, via make translate, but encountered an error, so I didn't push up any changes. Please take a look and see if you can round it out, @kushaldas.

If you'd prefer to follow up on the strings changes separately, then I'll say explicitly that the rest of the PR looks good to me!

Also updated the translations after updating the journalist
interface.
@kushaldas
Copy link
Contributor Author

@conorsch I just now pushed the change with journalist template update and also updated strings for translation.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for adding the string changes!

@conorsch conorsch merged commit 84b7042 into develop May 3, 2021
conorsch pushed a commit to freedomofpress/securedrop-docs that referenced this pull request May 4, 2021
Follow-up to freedomofpress/securedrop#5911,
in which we removed Xenial-specific codepaths from the code repo.
Here, we do the same throughout the documentation. There are notable
exceptions:

  * https://docs.securedrop.org/en/stable/v3_services.html
  * https://docs.securedrop.org/en/stable/upgrade/focal_migration.html

Those URLs remain untouched by these changes. We can follow up soon with
removal for those, too, but it seems helpful to maintain a bit longer.
conorsch pushed a commit to freedomofpress/securedrop-docs that referenced this pull request May 4, 2021
Follow-up to freedomofpress/securedrop#5911,
in which we removed Xenial-specific codepaths from the code repo.
Here, we do the same throughout the documentation. There are notable
exceptions:

  * https://docs.securedrop.org/en/stable/v3_services.html
  * https://docs.securedrop.org/en/stable/upgrade/focal_migration.html

Those URLs remain untouched by these changes. We can follow up soon with
removal for those, too, but it seems helpful to maintain a bit longer.
conorsch pushed a commit to freedomofpress/securedrop-docs that referenced this pull request May 4, 2021
Follow-up to freedomofpress/securedrop#5911,
in which we removed Xenial-specific codepaths from the code repo.
Here, we do the same throughout the documentation. There are notable
exceptions:

  * https://docs.securedrop.org/en/stable/v3_services.html
  * https://docs.securedrop.org/en/stable/upgrade/focal_migration.html

Those URLs remain untouched by these changes. We can follow up soon with
removal for those, too, but it seems helpful to maintain a bit longer.
@rmol rmol deleted the cancel_xenial branch June 23, 2021 13:50
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.

Remove all Xenial-specific code from the codebase
5 participants