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

Also run tests on Windows Server 2022 GitHub Runner #1176

Merged
merged 1 commit into from
Oct 1, 2021

Conversation

TBBle
Copy link
Contributor

@TBBle TBBle commented Sep 24, 2021

Low-hanging fruit, but nice to have in place while things like #1160 are in-flight.

Outstanding thoughts:

  • Make this a matrix job? Is it worth it for the two versions we care about until the mid-2020's?
  • While I'm on the topic, we probably should either test or explicitly declaim Server 2016 support. The README currently points to a system requirements doc that includes Server 2016... this solves itself in less than 4 months anyway.

@TBBle TBBle requested a review from a team as a code owner September 24, 2021 15:06
@dcantah
Copy link
Contributor

dcantah commented Sep 25, 2021

Somehow the CI is still showing a "test" check that is "Waiting for status to be reported" 😐

@TBBle
Copy link
Contributor Author

TBBle commented Sep 25, 2021

Weird. I wonder if that's because there's a test job in the mainline, and it doesn't like the idea of me removing tests like that. It's not 'CI / test' as it is without my change, for that matter.

I wonder if a matrix job would be better or be just as confusing for it.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
@TBBle
Copy link
Contributor Author

TBBle commented Sep 25, 2021

Drat. The matrix job was pretty easy (and gives a better diff, and more DRY), but it's still showing a mysterious 'test' job as required.

For that matter, I'm not sure what actually makes a particular step 'Required' here, but if that's actually a repository configuration rather than in .github/workflows/ci.yml (i.e. the same way DCO and license/cla are hooked up), then that would explain why test is appearing here even without such a job present in the YAML.

Edit: Poking around, it's probably a repository setting, the master branch will be set to "Require status checks to pass before merging", and it has the 'test' status check listed there. I suspect that list hasn't been updated as other CI stages have been added either, as I assume it currently lists "lint", "build", test", and "license/cla".

Edit again: Yeah, that's it, I can see the protections in the GitHub API.

> curl -H "Accept: application/vnd.github.v3+json" https://github.com/gitapi/repos/Microsoft/hcsshim/branches/master
...
  "protected": true,
  "protection": {
    "enabled": true,
    "required_status_checks": {
      "enforcement_level": "non_admins",
      "contexts": [
        "license/cla",
        "build",
        "lint",
        "test"
      ]
    }
  },
  "protection_url": "https://github.com/gitapi/repos/microsoft/hcsshim/branches/master/protection"
}

So trivially changing test to test (windows-2019) and test (windows-2022) would fix this (the settings UI lets you search in recent test run names). Or removing 'test' at least. It might be worth marking DCO as required too, and perhaps some of the other stages.

@anmaxvl
Copy link
Contributor

anmaxvl commented Sep 29, 2021

looks like we'd have to remove test from required checks, merge this PR and update the settings again? We could also use our administrator powers to merge this PR regardless the status of checks. what do you think @microsoft/containerplat ?

@TBBle
Copy link
Contributor Author

TBBle commented Sep 30, 2021

I could also add a test job that runs the Windows 2019 tests again, then we merge this, then change the GitHub config, and then a new MR that removes the (now-duplicate) test job. That feels pretty yucky though, and needlessly churny.

If I was making the decision, I'd force-accept this, and then change the GitHub config. We can see that the new test jobs have passed, after all. Although other PRs in-flight would then need to be rebased to pass the new checks, or similarly force-merged.

The ideal would be if GitHub could be told "test or test (windows-2019) is required", but I don't think it's that sophisticated.

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.

3 participants