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

ci: Add app test job using xenial #4035

Merged
merged 4 commits into from
Jan 16, 2019
Merged

Conversation

redshiftzero
Copy link
Contributor

Status

Ready for review

Description of Changes

Progress towards #4013, if there are no issues with the below then I will file followup issues for each class of app test failure

As mentioned in #4013, that bug was one of several issues that are surfaced by running the application test suite on Ubuntu Xenial.

In the next couple of weeks we are planning to make a lot of changes here in SecureDrop core to ensure that SecureDrop works as expected for Xenial. To support faster iterations and multiple developers to help out with this effort, I'm proposing in this PR that we add a CI job that runs the Xenial app tests on develop (in addition to the CI job that runs the Trusty app tests). This job is expected to fail until all app issues are resolved.

Why? Because this will be far, far faster than:

  1. running tests manually on Xenial to review
  2. everyone piling onto a single branch to try to resolve different issues while not stepping on each others toes
  3. maintaining a feature branch, which I think we should generally avoid such that we if there are other changes being made that will cause additional app test changes to be made, we don't discover them at time of merge of the feature branch into develop

Thoughts?

Testing

  1. Are there now two CI jobs running here: xenial-app-tests and trusty-app-tests, where trusty passes but xenial does not?

  2. Can you now make test-xenial locally and run tests, that will fail?

Deployment

This is CI and dev env only

Checklist

If you made non-trivial code changes:

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

As evidenced by the test failures we've discovered, we'll need
to make some changes that ideally work under both Trusty and Xenial.
This means that we need to run the application tests in both
environments. This PR adds a $BASE_OS env var that can be either
"trusty" or "xenial". It uses this env var for docker builds, and modifies
the docker build logic to point to a Dockerfile outside of the build
context [1]. We make a new directory in the securedrop/ directory
called Dockerfiles that contains the Dockerfiles for both
environments.

By default, the other Makefile targets will use
Trusty. Once trusty is EOL, we can delete the Trusty
Dockerfile, though it would be prudent to leave the BASE_OS
logic in place for the next major OS transition.

[1] docker/cli#886
@redshiftzero redshiftzero force-pushed the xenial-pgp-journalist branch from ca9a9a1 to d927f18 Compare January 16, 2019 04:17
Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

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

This works locally according to the ticket. I'm in support of adding failing CI jobs here as the easiest way forward for the moment. Waiting on 0thers' input to merge.

@redshiftzero
Copy link
Contributor Author

OK after discussion in standup, we're going to indeed merge this CI job and then fix the test failures iteratively (cc @kushaldas @emkll), thanks for review @heartsucker!

For the future, the trusty-app-tests job is required for merge, but not the xenial-app-tests job. PRs made before this PR will have the trusty-app-tests job instead called tests, if that job is passing and you want to merge the PR, you can @ me in the thread to merge the PR if you don't have permissions (avoids asking contributors to rebase, which is annoying).

@redshiftzero redshiftzero merged commit e279856 into develop Jan 16, 2019
@redshiftzero redshiftzero deleted the xenial-pgp-journalist branch January 16, 2019 23:08
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.

2 participants