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

Runs fetch-tor-packages with pinned version in CI #4300

Merged
merged 1 commit into from
Apr 1, 2019

Conversation

conorsch
Copy link
Contributor

Status

Ready for review.

Description of Changes

Closes #4170.

Adds a tor_version var to the fetch-tor-packages logic. This is yet another point of update when we bump versions, but pinning will help us avoid surprises. To that end, we're also running the fetch action in CI now, to help us catch version mismatches earlier.

Testing

  • Confirm make fetch-tor-packages passes locally
  • Confirm CI is passing

Deployment

No concerns, dev env tooling only. There's a modest improvement in reliability in the release window, ensuring we fetch the same package version we've been testing.

Checklist

If you made non-trivial code changes:

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

@emkll emkll force-pushed the 4170-run-fetch-tor-packages-in-ci branch from b8b4371 to b99ce8e Compare March 28, 2019 21:39
emkll
emkll previously requested changes Mar 29, 2019
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

I've rebased this branch on latest develop to address linting container issues (#4301), and proceeded with the following test plan:

Testing

  • Confirm make fetch-tor-packages passes locally
  • ❌ Confirm CI is passing

In the current, it seems like the venv isn't preserved across ssh sessions: https://circleci.com/gh/freedomofpress/securedrop/25033

Wrapping the molecule command in a shell script and bootstrapping the venv (285d4df4f1c7) seems to get us further.

However, there's another failure that is likely due to the fact we are rsyncing of the repo to the gce instance instead of cloning it, as we do in workstations: https://github.com/freedomofpress/securedrop/blob/develop/devops/gce-nested/gce-runner.sh#L40 see https://circleci.com/gh/freedomofpress/securedrop/25040

Perhaps we can sidestep by using it locally inside the circle container (as part of this or another job) and not via ssh_gce?

@conorsch
Copy link
Contributor Author

Great comments, @emkll. I agree a separate job would be better for a few reasons, including parallel execution. Pushed a trial commit based on your examples, will follow up once we have feedback from the CI system. Marking as WIP for now, until we confirm CI is passing.

Adds a `tor_version` var to the fetch-tor-packages logic.
This is yet another point of update when we bump versions,
but pinning will help us avoid surprises. To that end, we're
also running the fetch action in CI now, to help us catch
version mismatches earlier.

Using a separate CI job so as not to add to the serial execution
of the large staging job.
@conorsch conorsch force-pushed the 4170-run-fetch-tor-packages-in-ci branch from 2f2ac01 to a90f731 Compare March 29, 2019 17:08
@conorsch
Copy link
Contributor Author

CI passed on the fetch tor packages logic here: https://circleci.com/gh/freedomofpress/securedrop/25052 So I've squashed the commits. Once you confirm CI is passing here, @emkll, I'd appreciate a sanity check that the make fetch-tor-packages still works for you reliably. I pulled in your suggested logic on the script breakage, and it works for me.

@emkll emkll dismissed their stale review March 29, 2019 20:49

Confirm fetch logic working for me locally, unfortunately since kernels have been uploaded to apt-test, #4308 needs to be merged in first and this branch rebased on top of that commit.

@conorsch
Copy link
Contributor Author

conorsch commented Apr 1, 2019

Re-running CI after merge of #4308.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

LGTM:

  • Confirm make fetch-tor-packages passes locally
  • Confirm CI is passing
  • Theres a new check for fetch-tor-debs

@emkll emkll merged commit bf4a2ac into develop Apr 1, 2019
@emkll emkll deleted the 4170-run-fetch-tor-packages-in-ci branch April 1, 2019 21:25
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