Skip to content

ci: init get-merge-commit workflow#361494

Merged
infinisil merged 3 commits intoNixOS:masterfrom
JohnRTitor:ci-merge-commit
Dec 4, 2024
Merged

ci: init get-merge-commit workflow#361494
infinisil merged 3 commits intoNixOS:masterfrom
JohnRTitor:ci-merge-commit

Conversation

@JohnRTitor
Copy link
Member

Since we are preferring to use get-merge-commit.sh to resolve the merge commit, let's make it a reusable workflow so we can use it in other actions (ie, in #361447)

Tested at JohnRTitor#2

@JohnRTitor JohnRTitor requested review from Mic92 and infinisil December 3, 2024 17:23
@github-actions github-actions bot added 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Dec 3, 2024
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Dec 3, 2024
@nix-owners nix-owners bot requested a review from philiptaron December 3, 2024 17:40
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Very nice!

Copy link
Member

Choose a reason for hiding this comment

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

Do we need a separate job for this? It seems like this can just be a separate step in the first job instead. This way we wouldn't spam the list of CI checks with many of these

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to move it after the checkout, as the message suggests. Otherwise it hasn't cloned the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

On Matrix (since there were problems with GitHub) @JohnRTitor shared this:

Unlike when you are using actions within a workflow, you call reusable workflows directly within a job, and not from within job steps.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks like that works best with a separate repository though, which we could totally do by requesting it, won't be as local anymore though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Composite actions does not need a separate repo, I use them in both in-repo and out-of-repo.

In-repo example https://github.com/azuwis/nix-config/blob/dd9650cce55ca08d1304fb49e850a4ddaeb42f8d/.github/workflows/package.yml#L53

We can totally put them in the ci/ dir.

@JohnRTitor JohnRTitor marked this pull request as draft December 3, 2024 17:55
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Dec 4, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what I am doing wrong here. Tried my best to follow: https://github.com/storacha/add-to-web3/blob/main/action.yml

which gives an output as well.

https://github.com/JohnRTitor/nixpkgs/actions/runs/12164535023/job/33926420934?pr=2 currently does not run at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

CC @azuwis

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be better to have this as a seperate workflow, in a seperate repo.

Or we can just revert to the previous solution.

Copy link
Member

Choose a reason for hiding this comment

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

@JohnRTitor I believe the error is accurate, the repo needs to be checked out before a local workflow can be used. So this part can't be reused and needs to be duplicated:

    - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
      with:
        path: base
        sparse-checkout: ci

Copy link
Member

Choose a reason for hiding this comment

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

Discussed on Matrix. Main points:

  • This then also requires rm -rf base again, kind of annoying
  • @JohnRTitor had the idea of turning eval and Nixpkgs-vet to a reusable workflow and call it from a bigger/main workflow, to explore in the future
  • For now we can just use reusable workflows, it's not too much extra noise and we can always change it

Copy link
Member Author

Choose a reason for hiding this comment

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

Sending this for review, but looks like GitHub is having some issues with CI.

@JohnRTitor JohnRTitor marked this pull request as ready for review December 4, 2024 19:11
@JohnRTitor JohnRTitor requested a review from infinisil December 4, 2024 19:11
Copy link
Member

Choose a reason for hiding this comment

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

Security concern: There's no permissions specified here, so this might use a default token with a lot of permissions and not the one from the parent workflow with little permissions. Should definitely specify permissions here to limit it to only what's necessary (which requires figuring out which one this uses). See also https://docs.github.com/en/actions/sharing-automations/reusing-workflows#using-inputs-and-secrets-in-a-reusable-workflow

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to go with:

# We need a token to query the API, but it doesn't need any special permissions
permissions: {}

ci/README.md Outdated
Comment on lines 86 to 90
Copy link
Member

Choose a reason for hiding this comment

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

We can get rid of this part.

Signed-off-by: John Titor <50095635+JohnRTitor@users.noreply.github.com>
@infinisil infinisil merged commit 93ef541 into NixOS:master Dec 4, 2024
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Dec 4, 2024

Git push to origin failed for release-24.11 with exitcode 1

@infinisil
Copy link
Member

Follow-up: #364338

@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants