Skip to content

Conversation

@chrisgavin
Copy link
Contributor

The current Python CI jobs seem to assume the bundle will never be updated by referencing the bundle by exact version. They don't specify a static bundle to actually download though, so every time the bundle updates the CI will break.

I've fixed this by specifying the expected bundle as the one to download under the assumption that is what we want to test. We could also test the latest version, but that seems like it could break unexpectedly if there are changes to the Python extractor.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@robertbrignull
Copy link
Contributor

Is the issue here that it's using the bundle from the virtual environment instead of whatever is the latest for this branch? Why did that cause a failure? Can you link to the logs as I didn't spot any failures?

Note there's another job in this workflow. You've only changed the job for windows. I'd expect both want to be kept in sync.
I haven't really thought it through but I'd argue that perhaps all the CI jobs in this repo should be specifying "latest".

Also cc. @Daverlo on this PR

@chrisgavin
Copy link
Contributor Author

Is the issue here that it's using the bundle from the virtual environment instead of whatever is the latest for this branch? Why did that cause a failure?

It does neither of those. It always uses the codeql-bundle-20200826 bundle. It's a hard-coded string in

py -3 $Env:GITHUB_WORKSPACE\\python-setup\\auto_install_packages.py C:\\hostedtoolcache\\windows\\CodeQL\\0.0.0-20200826\\x64\\codeql
.

Can you link to the logs as I didn't spot any failures?

https://github.com/github/codeql-action/actions/runs/328696002

Note there's another job in this workflow. You've only changed the job for windows. I'd expect both want to be kept in sync.
I haven't really thought it through but I'd argue that perhaps all the CI jobs in this repo should be specifying "latest".

The other one always uses the version from the virtual environment so it doesn't break as easily (though it could still break arbitrarily when the environment is updated). It does seem like probably both the jobs are wrong, but only one is broken.

@chrisgavin chrisgavin force-pushed the fix-ci branch 2 times, most recently from ceec3d0 to 548e178 Compare October 26, 2020 14:12
@chrisgavin
Copy link
Contributor Author

I've updated both the Windows and Linux tests to always use the latest bundle. I think this is probably the best thing to test because it ensures new bundles work before we start using them rather than after. It also means tests won't break when the environment updates.

Copy link
Contributor

@Daverlo Daverlo left a comment

Choose a reason for hiding this comment

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

Thanks! I forgot to remove the hardcoded version from the windows workflow.

The other one always uses the version from the virtual environment so it doesn't break as easily (though it could still break arbitrarily when the environment is updated). It does seem like probably both the jobs are wrong, but only one is broken.

Could you explain this a bit more please? I though the linux job was correct

@chrisgavin
Copy link
Contributor Author

chrisgavin commented Oct 26, 2020

The Linux job previously used whatever CodeQL bundle was included in the Actions environment it was running in, meaning:

  1. Whenever the CodeQL bundle is updated, the new bundle is not tested against the Python setup scripts.
  2. If the environment is updated with an incompatible CodeQL bundle the CI on this repository will break.

Both of those seem like undesirable properties.

@Daverlo
Copy link
Contributor

Daverlo commented Oct 27, 2020

If the environment is updated with an incompatible CodeQL bundle the CI on this repository will break.

This is the part that I'm not understanding. How would the environment be updated?

@chrisgavin
Copy link
Contributor Author

chrisgavin commented Oct 27, 2020

The default CodeQL Bundle comes from the Actions virtual environments. They are periodically updated, getting their bundle from whatever is the latest bundle referenced in main. So what would happen is:

  1. Someone updates the CodeQL Bundle with a pull request against main. This would previously be untested because the tests use the CodeQL Bundle from the environment, not the one referenced in main.
  2. A new virtual environment is built using the new bundle. This is also untested because the CodeQL Action CI does not get run on virtual environment updates.
  3. The new virtual environment is rolled out on ring 0 and the tests now start trying to use it for all new PRs. The best case is it's a problem with the tests and the CI on this repository is failing until the problem is fixed. The worst case is it's a problem with the new bundle that no tests were run for until after we started rolling it out.

@Daverlo
Copy link
Contributor

Daverlo commented Oct 27, 2020

I knew there were some changes in the process of releasing a new bundle, but I wasn't aware all this amazing work was done and now we are using the Actions virtual environments. That explains it, thanks!

Lets merge this?

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