Skip to content

Conversation

@chrisgavin
Copy link
Contributor

@chrisgavin chrisgavin commented Oct 30, 2020

This adds a warning, logged by the API client if the Action detects it is communicating with an unsupported version of GitHub Enterprise Server.

For the purposes of this check, a version is supported if all the following are true:

  • It's greater than 2.22.0 (the first version to support Code Scanning).
  • It's not end-of-life.
  • It's feature freeze is not more than 2 weeks away.

This means new versions become supported two weeks from being feature frozen, and support is dropped one year after release. The supported versions are updated automatically by a nightly cron job that will open a pull request at the right times to add support for new versions or remove support for old versions.

This allows us to make breaking changes to the APIs used by the Action whilst still providing good feedback to users about how to ensure their workflows keep working on GHES.

Merge / deployment checklist

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

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Interesting you've gone for a warning approach instead of outright stopping people. I have no comments on this.

Do we need approval for the references to the enterprise/releases repo? Is linking to internal resources like that the best way to achieve this?

@chrisgavin
Copy link
Contributor Author

Interesting you've gone for a warning approach instead of outright stopping people.

Yes, there's a few benefits to doing a warning. It eases development and testing of new GHES versions as the Action won't fail until we officially add support for the new version. It also means if we happen not to break support for an older release we won't cause any unnecessary issues with people using GitHub Connect.

It does have the disadvantage that we have to be a bit careful that we don't change the semantics of an API without actively breaking it, but that seems like a fairly small risk and we already have to think pretty carefully about API changes to avoid breaking supported versions.

Do we need approval for the references to the enterprise/releases repo? Is linking to internal resources like that the best way to achieve this?

I don't really see an issue with it as the only information that gets pulled in by the update script in the end is really just the feature freeze date for the next version. @rneatherway and I spent a good amount of time trying to think of an alternative but we couldn't come up with anything other than updating it manually for each release which seemed like it could be a bit error-prone. I've asked whether this is okay and got a non-committal but vaguely positive answer.

@robertbrignull
Copy link
Contributor

Yes, there's a few benefits to doing a warning. It eases development and testing of new GHES versions as the Action won't fail until we officially add support for the new version. It also means if we happen not to break support for an older release we won't cause any unnecessary issues with people using GitHub Connect.

It does have the disadvantage that we have to be a bit careful that we don't change the semantics of an API without actively breaking it, but that seems like a fairly small risk and we already have to think pretty carefully about API changes to avoid breaking supported versions.

It's nice that it might just work for unsupported versions, and I agree given how much we're changing things and how careful we're being there's a good chance it'll still work.

My main worry with just a warning is that people won't see it in the logs and thus there will still be the cost of a support request to discover the problem. I guess one things that works in our favour here is that the warning is printed after the request, so if the request fails then the version warning should be the last thing that gets printed. So considering that, I'm happy with this implementation.

@rneatherway
Copy link
Contributor

I agree with Robert, so approving.

@robertbrignull
Copy link
Contributor

I had one other comment I made (#285 (comment)) which I think would be good to consider, but that could be pushed to later if it's more important to get this in now.

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@chrisgavin chrisgavin merged commit 1364a74 into main Nov 3, 2020
@chrisgavin chrisgavin deleted the check-api-version branch November 3, 2020 13:36
@github-actions github-actions bot mentioned this pull request Nov 9, 2020
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