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

ci: copy new Unified CI templates and bump go.mod to Go 1.20 #2471

Merged
merged 14 commits into from
Aug 19, 2023

Conversation

web3-bot
Copy link
Collaborator

This PR was created automatically by the @web3-bot as a part of the Unified CI project.

@galargh galargh self-assigned this Aug 11, 2023
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Can we please get the version number back? It's not clear what "this" and "next" means:
image

Why is there no log?
image

on:
pull_request:
push:
branches: ["master","release-v018","release-v022","release-v023","release-v026","release-v027","release-v028","release-v029"]
Copy link
Contributor

Choose a reason for hiding this comment

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

How did these end up here? This is just a convention, but we might do something different in the future. It also means we'd have to update workflow files for every release.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to stop running the workflows twice on pull requests. We can use our branch protection rule pattern here. For @web3-bot to be able to retrieve it, we'd need a higher level access (gh api graphql -f query='query($owner: String!, $repo: String!) { repository(owner: $owner, name: $repo) { branchProtectionRules(first: 100) { nodes { pattern } } } }' -f owner=libp2p -f repo=go-libp2p --jq '.data.repository.branchProtectionRules.nodes').

We can update it once to:

Suggested change
branches: ["master","release-v018","release-v022","release-v023","release-v026","release-v027","release-v028","release-v029"]
branches: ["master","release-v0[0-9][0-9]"]

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to stop running the workflows twice on pull requests.

Wasn't there a subtle difference between push and pull_request? One of them creates a fake merge commit, doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to stop running the workflows twice on pull requests.

Wasn't there a subtle difference between push and pull_request? One of them creates a fake merge commit, doesn't it?

Yes, there is. pull_request creates a fake merge commit to the base branch of the PR (so it tests the "what if I merged this PR right now"), and push checks out the head branch as is.

To maintain the old behaviour we could get rid of the branches filter altogether:

Suggested change
branches: ["master","release-v018","release-v022","release-v023","release-v026","release-v027","release-v028","release-v029"]

After the updates to Unified CI I'm rolling out now, @web3-bot will start respecting changes like this in future rollouts, so we don't have to worry about it being overridden in the future.

The reason why I decided to add the branches filter in the first place is that it's a saner default for all repositories but diverging from that default is definitely OK.

Copy link
Contributor

@galargh galargh 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 the prompt review 🙇 There's one bigger issue that this push has discovered 😿

Called workflows cannot be queued onto self-hosted runners across organisations/enterprises. Failed to queue this job. Labels: 'self-hosted , linux , x64 , 2xlarge'.

Apparently, we cannot use self-hosted runners from reusable workflows defined in a different organisation. I'll need to think a little bit about how to handle it best.

files: '${{ env.COVERAGES }}'
env_vars: OS=${{ matrix.os }}, GO=${{ matrix.go }}
go-test:
uses: pl-strflt/uci/.github/workflows/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please get the version number back? It's not clear what "this" and "next" means:

this refers to the Go version defined in the go.mod file, and next refers to the next version (minor + 1). Refering to versions like this allows us to bump Go without having to touch workflows.

However, we can go opt out of this and go back to the old ways with:

Suggested change
uses: pl-strflt/uci/.github/workflows/[email protected]
uses: pl-strflt/uci/.github/workflows/[email protected]
with:
go-versions: '["1.19.x", "1.20.x"]'

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting idea! That would mean that we could just bump Go versions by editing go.mod, right? That would be very convenient for go-libp2p.

However, how does this work for less frequently used repos? It would be nice if we could still keep them up to date.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting idea! That would mean that we could just bump Go versions by editing go.mod, right? That would be very convenient for go-libp2p.

Yes! Exactly!

However, how does this work for less frequently used repos? It would be nice if we could still keep them up to date.

@web3-bot is still going to take care of it. The difference is that it's going to happen in a separate PR that touches only Go files and not CI workflows. And if anyone does it themselves before web3-bot gets to it, then it's just going to leave it intact :)

on:
pull_request:
push:
branches: ["master","release-v018","release-v022","release-v023","release-v026","release-v027","release-v028","release-v029"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to stop running the workflows twice on pull requests. We can use our branch protection rule pattern here. For @web3-bot to be able to retrieve it, we'd need a higher level access (gh api graphql -f query='query($owner: String!, $repo: String!) { repository(owner: $owner, name: $repo) { branchProtectionRules(first: 100) { nodes { pattern } } } }' -f owner=libp2p -f repo=go-libp2p --jq '.data.repository.branchProtectionRules.nodes').

We can update it once to:

Suggested change
branches: ["master","release-v018","release-v022","release-v023","release-v026","release-v027","release-v028","release-v029"]
branches: ["master","release-v0[0-9][0-9]"]

@galargh
Copy link
Contributor

galargh commented Aug 11, 2023

go: command not found

There's no default Go installed on self-hosted runners. I'm on it. Will put it in draft as I resolve all the issues.

@galargh galargh marked this pull request as draft August 11, 2023 13:04
@galargh galargh closed this Aug 11, 2023
@galargh galargh reopened this Aug 11, 2023
@galargh galargh closed this Aug 11, 2023
@galargh galargh reopened this Aug 11, 2023
@galargh galargh closed this Aug 11, 2023
@galargh galargh reopened this Aug 11, 2023
@galargh
Copy link
Contributor

galargh commented Aug 11, 2023

Alright! It's ready for review again. I was able to resolve the issue with using self-hosted runners from reusable workflows across organizations (see #2471 (comment) for details).

Before we can proceed with the merge, we have 2 conversations that should be resolved:

What should the jobs of Go test workflow be called?

  1. Option 1: ubuntu (go this), ubuntu (go next), ...
  2. Option 2: ubuntu (go 1.20.x), ubuntu (go 1.21.x), ...

The advantage of Option 1 is that we won't have to touch Go test workflow when we upgrade Go version in go.mod. Also, it makes it possible to mark Go test workflow jobs as required checks since the names are stable.

What triggers should we use for Go test and Go check workflows?

We cannot leave the branches filter as-is because it's unmaintainable.

  1. Option 1: Remove branches filter entirely
  2. Option 2: Set branches filter to ["master","release-v0[0-9][0-9]"]

Option 1 ensures that each PR is tested both as-is and as-if it was merged. Option 2, on the other hand, reduces the number of triggered workflows.


@marten-seemann Please let me know what you think 😄 As soon as we resolve these, I'll use @web3-bot to update Go version used in this repo.

@galargh galargh marked this pull request as ready for review August 11, 2023 15:55
@marten-seemann
Copy link
Contributor

Before we can proceed with the merge, we have 2 conversations that should be resolved:

What should the jobs of Go test workflow be called?

  1. Option 1: ubuntu (go this), ubuntu (go next), ...
  2. Option 2: ubuntu (go 1.20.x), ubuntu (go 1.21.x), ...

I think it pays off to be explicit here (i.e. option 2). I've run into quite a lot of situations where builds failed with one Go version, but not the other. Having another level of mental indirection here makes debugging more difficult. Is there any way to dynamically set the name of the job?

ymmv, but I don't care much about required checks. We don't merge any PR that has any CI failure, so there's little value in explicitly marking any particular check as required. They all are (implicitly).

@galargh
Copy link
Contributor

galargh commented Aug 14, 2023

Is there any way to dynamically set the name of the job?

Unfortunately not, we can only set the name before the job starts. We could parse the version strings in a another job that runs before all the others, but I think that would be a waste of a job.

I restored the explicit setting of go-versions for the go-test job. This should reflect in the checks shortly.

I also updated the push triggers to reflect the protected branches' patterns. If you want me to remove the filter entirely, let me know.

@galargh
Copy link
Contributor

galargh commented Aug 14, 2023

I added the Go version bump to this branch too not to delay the process :)

@galargh
Copy link
Contributor

galargh commented Aug 14, 2023

Go Checks is failing because go fix that we run as part of the Go version bump modified proto generated code. I'll revert the changes to the generated code. Why the proto generate code is not go fix compliant is a whole different story.

@galargh
Copy link
Contributor

galargh commented Aug 14, 2023

And I updated quic-go with go get -u github.com/quic-go/quic-go. Disclaimer, I haven't looked if any additional changes are required.

@galargh
Copy link
Contributor

galargh commented Aug 14, 2023

As discussed with @marten-seemann, I reverted the Go version upgrade changes because they're being worked on separately.

@galargh galargh changed the title ci: uci/copy-templates Copy new Unified CI templates and bump go.mod to Go 1.20 Aug 18, 2023
@galargh galargh requested a review from MarcoPolo August 18, 2023 10:30
@marten-seemann marten-seemann changed the title Copy new Unified CI templates and bump go.mod to Go 1.20 ci: copy new Unified CI templates and bump go.mod to Go 1.20 Aug 19, 2023
@marten-seemann marten-seemann merged commit 4a0c385 into master Aug 19, 2023
11 checks passed
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