Skip to content

Make PR checks compatible with the latest version of the ubuntu-latest runner image #1330

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.

Sign up for GitHub

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

Jump to bottom
Merged
henrymercer merged 6 commits into from
Oct 31, 2022
Merged

Make PR checks compatible with the latest version of the ubuntu-latest runner image #1330

henrymercer merged 6 commits into from
Oct 31, 2022

Conversation

@henrymercer
Copy link
Contributor

@henrymercer henrymercer commented Oct 27, 2022 •
edited
Loading

This PR updates the PR checks to handle the recent update to the ubuntu-latest runner image. This is being moved over to Ubuntu 22.04, which is only compatible with CodeQL CLI 2.7.3 and later. Some of our PR checks use very old CLIs, and this PR updates them to run on the ubuntu-20.04 image instead.

Planned work:

  • Introducing a process to run our checks against new runner images before the -latest image changes.
  • In the future, deprecating support for versions of the CLI earlier than 2.7.3.
  • Potentially introducing a more helpful error message into the Action when we hit this case. Impact depends on how many customers are running the latest Action (so they'll benefit from the new error message) against a very old CLI (older than 2.7.3).

Merge / deployment checklist

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

@henrymercer henrymercer force-pushed the henrymercer/ubuntu-image-upgrade branch 2 times, most recently from fff7f04 to 69d3a56 Compare October 27, 2022 14:00
@henrymercer
Run PR checks for CLIs < 2.7.3 on ubuntu-20.04
f248a57 
Build tracing using CLIs before 2.7.3 no longer works with the most
recent update to the `ubuntu-22.04` runner image.

With this new logic, we can remove the workarounds around testing
`windows-2019` and `windows-2022`.
@henrymercer
Remove redundant fail-fast configurations
5767f91
@henrymercer henrymercer force-pushed the henrymercer/ubuntu-image-upgrade branch from 69d3a56 to d105e6f Compare October 27, 2022 14:19
@henrymercer henrymercer changed the title Disable fail-fast in PR checks Update PR checks for compatibility with the latest version of the ubuntu-latest runner image Oct 27, 2022
@henrymercer henrymercer changed the title Update PR checks for compatibility with the latest version of the ubuntu-latest runner image Make PR checks compatible with the latest version of the ubuntu-latest runner image Oct 27, 2022
@henrymercer henrymercer force-pushed the henrymercer/ubuntu-image-upgrade branch from d105e6f to 8fdcf29 Compare October 27, 2022 15:04
@henrymercer henrymercer force-pushed the henrymercer/ubuntu-image-upgrade branch from 8fdcf29 to 4ed5abe Compare October 27, 2022 15:23
aeisenberg
aeisenberg reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

@henrymercer henrymercer marked this pull request as ready for review October 27, 2022 17:18
@henrymercer henrymercer requested a review from a team as a code owner October 27, 2022 17:18
aeisenberg
aeisenberg reviewed Oct 27, 2022
View reviewed changes
.github/workflows/unset-environment-new-cli.yml
- os: ubuntu-latest
- os: ubuntu-20.04
version: stable-20210809
- os: ubuntu-latest
Copy link
Contributor

@aeisenberg aeisenberg Oct 27, 2022

Choose a reason for hiding this comment

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

Should this be changed to ubuntu-22.04? Or maybe above should be changed to ubuntu-latest (I think I'd prefer this). In general, we should not have to specify 22.04 (using latest instead) and when the next version of ubuntu comes out, some of our tests may break, but that's a good thing since we will be aware of this and can fix proactively.

Copy link
Contributor Author

@henrymercer henrymercer Oct 28, 2022

Choose a reason for hiding this comment

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

Could you clarify what you mean by "Or maybe above should be changed to ubuntu-latest"?

Copy link
Contributor

@aeisenberg aeisenberg Oct 28, 2022

Choose a reason for hiding this comment

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

Sorry...that wasn't very clear. I am suggesting that everywhere there is ubuntu-22.04 now, we should change to ubuntu-latest. This was when ubuntu-latest is changed again, we will know if anything is broken.

In particular, I was thinking here:

https://github.com/github/codeql-action/pull/1330/files/993ca05cd7741fd4e48fe6b8a3b28f102174f4b6#diff-9bec90e4ad9a179ec1c8e1266c2ddc2fc0fbd73db55720b3425cfdf667b89e14R116

and here:

https://github.com/github/codeql-action/pull/1330/files/993ca05cd7741fd4e48fe6b8a3b28f102174f4b6#diff-9bec90e4ad9a179ec1c8e1266c2ddc2fc0fbd73db55720b3425cfdf667b89e14R116

And in other places in .github/workflows/python-deps.yml.

Copy link
Contributor Author

@henrymercer henrymercer Oct 28, 2022

Choose a reason for hiding this comment

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

Ah, I see what you mean now. I think you're generally right, but my hesitancy for python-deps.yml specifically is that these jobs were previously testing against both ubuntu-20.04 and ubuntu-22.04, and the migration of ubuntu-latest to ubuntu-22.04 isn't fully rolled out. It would be possible for ubuntu-latest to start pointing to ubuntu-20.04 again before the rollout completes, which would cause us to silently lose test coverage here. I'd be in favour to switching to ubuntu-latest once the rollout completes.

Copy link
Contributor

@aeisenberg aeisenberg Oct 28, 2022

Choose a reason for hiding this comment

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

I'd be in favour to switching to ubuntu-latest once the rollout completes.

Sure. I'm guessing you're tracking this in https://github.com/github/codeql-core/issues/2909?

@angelapwen angelapwen mentioned this pull request Oct 27, 2022
Merged
3 tasks
aeisenberg
aeisenberg approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

This is fine as long as we use https://github.com/github/codeql-core/issues/2909 to remember to move to ubuntu-latest when 22.04 is fully rolled out.

@henrymercer henrymercer merged commit 7e25850 into main Oct 31, 2022
@henrymercer henrymercer deleted the henrymercer/ubuntu-image-upgrade branch October 31, 2022 10:07
This was referenced Nov 2, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeisenberg aeisenberg aeisenberg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@henrymercer @aeisenberg