Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: modify preview deployment script to work with forks#13404

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
dannykopping wants to merge3 commits intocoder:mainfromdannykopping:dk/fix-preview

Conversation

dannykopping
Copy link
Contributor

@dannykoppingdannykopping commentedMay 30, 2024
edited
Loading

I maintain a fork of coder atdannykopping/coder.

When a branch exists in a fork only, the current preview deployment scripts break.
This PR modifies the script & action to reference thecoder/coder repo explicitly for the PR.

I also changed topr status becausepr view does not work with--repo.

… coder/coder repoSigned-off-by: Danny Kopping <danny@coder.com>
@dannykoppingdannykopping changed the titleModify preview deployment script to work with forkschore: modify preview deployment script to work with forksMay 30, 2024
@matifali
Copy link
Member

matifali commentedMay 30, 2024
edited
Loading

Does this make it work with forks and community contributors? We are using a few repo secrets in the deployment workflow, and I am not sure if this workflow allows forks to use the secret values.

@dannykopping
Copy link
ContributorAuthor

Does this make it work with forks and community contributors? We are using a few repo secrets in the deployment workflow, and I am not sure if this workflow allows forks to use the secret values.

I don't think so (at least that was not my intent), it just modified so that PRs created using forks would work.
I'm adding a clause to the action that only members of thecoder org can trigger this action.

matifali reacted with thumbs up emoji

Signed-off-by: Danny Kopping <danny@coder.com>
- name: Check if actor is a member
run: |
set -euo pipefail
response=$(curl -s -o /dev/null -w "%{http_code}" -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" "https://api.github.com/orgs/coder/members/${{ github.actor }}")
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this works in each case? For example,https://api.github.com/orgs/coder/members/matifali returns

{"message":"User does not exist or is not a public member of the organization","documentation_url":"https://docs.github.com/rest/orgs/members#check-public-organization-membership-for-a-user"}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It seems like it works for private members:

$ curl -s -H"Authorization: token$(gh auth token)""https://api.github.com/orgs/coder/members/matifali" -v ...< HTTP/2 204 ...

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

matifali reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Yes, all works great. I was testing without the token and always getting a 302. Thank you for adding the check.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to test by deploying the current PR.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I tried that in#13280 (where I originally had this fix) but GH didn't seem to use the new workflow. Is there a timeout maybe? I'll give it a try though.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Damn, I tried to execute this but I got:

$ ./scripts/deploy-pr.sh -d -b -y                                                                           branchName: dk/fix-previewprNumber: 13404experiments: build:truedeploy:truecould not create workflow dispatch event: HTTP 422: No ref found for: dk/fix-preview (https://api.github.com/repos/coder/coder/actions/workflows/60960476/dispatches)

I think the branch has to be in the main repo.
Strange that I didn't get this before, though.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Oh, looks like I must've pusheddk/verify-agent (the branch I was testing with previously) to themain repo somehow?

Copy link
Member

@matifalimatifaliMay 30, 2024
edited
Loading

Choose a reason for hiding this comment

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

I am noticing the same issue. It's deploying the previous branch I was working on.node-20
#13404 (comment)

@dannykopping
Copy link
ContributorAuthor

I've opened#13405 to validate.

matifali reacted with heart emoji

@matifali
Copy link
Member

matifali commentedMay 30, 2024
edited
Loading

Thank you. This is a good improvement. I plan to make this flow better sometimes#11331

@matifali
Copy link
Member

I tried deploying this PR but its detecting the wrong branch and PR number

gh pr checkout 13404remote: Enumerating objects: 12, done.remote: Counting objects: 100% (12/12), done.remote: Compressing objects: 100% (2/2), done.remote: Total 12 (delta 10), reused 12 (delta 10), pack-reused 0Unpacking objects: 100% (12/12), 1.71 KiB | 877.00 KiB/s, done.From https://github.com/coder/coder * [new ref]             refs/pull/13404/head -> dk/fix-previewSwitched to branch 'dk/fix-preview'git statusOn branch dk/fix-previewnothing to commit, working tree clean./scripts/deploy-pr.shAre you sure you want to deploy? (y/n) ybranchName: node-20prNumber: 12921experiments:build: falsedeploy: false✓ Created workflow_dispatch event for pr-deploy.yaml at node-20To see runs for this workflow, try: gh run list --workflow=pr-deploy.yaml

@dannykopping
Copy link
ContributorAuthor

I tried deploying this PR but its detecting the wrong branch and PR number

gh pr checkout 13404remote: Enumerating objects: 12, done.remote: Counting objects: 100% (12/12), done.remote: Compressing objects: 100% (2/2), done.remote: Total 12 (delta 10), reused 12 (delta 10), pack-reused 0Unpacking objects: 100% (12/12), 1.71 KiB | 877.00 KiB/s, done.From https://github.com/coder/coder * [new ref]             refs/pull/13404/head -> dk/fix-previewSwitched to branch 'dk/fix-preview'git statusOn branch dk/fix-previewnothing to commit, working tree clean./scripts/deploy-pr.shAre you sure you want to deploy? (y/n) ybranchName: node-20prNumber: 12921experiments:build: falsedeploy: false✓ Created workflow_dispatch event for pr-deploy.yaml at node-20To see runs for this workflow, try: gh run list --workflow=pr-deploy.yaml

Weird, it works for me:

$ gh pr status --repo=coder/coder --json headRefName,number --jq'.createdBy[0]'| cat{"headRefName":"dk/fix-preview","number":13404}

@matifali
Copy link
Member

matifali commentedMay 30, 2024
edited
Loading

Strangely for me

gh pr view --json headRefName | jq -r .headRefNamedk/fix-previewgh pr view --json number | jq -r .number13404

but

gh pr status --repo=coder/coder --json headRefName,number --jq '.createdBy[0]' | cat{"headRefName":"node-20","number":12921}

@dannykopping
Copy link
ContributorAuthor

Looking into it,pr list seems like a better option.

@dannykopping
Copy link
ContributorAuthor

Can you try this@matifali?

$ gh pr list --repo=coder/coder -H$(git rev-parse --abbrev-ref HEAD) --json number --jq'.[].number'| cat13404
matifali reacted with thumbs up emoji

@dannykopping
Copy link
ContributorAuthor

Alternatively we should skip the convenience and just accept a PR number as an argument.

@matifali
Copy link
Member

gh pr list --repo=coder/coder -H $(git rev-parse --abbrev-ref HEAD) --json number --jq '.[].number' | cat
13404
Also
gh pr status --json headRefName,number --jq '.currentBranch' | cat
{"headRefName":"dk/fix-preview","number":13404}

@dannykopping
Copy link
ContributorAuthor

I don't trustpr status anymore, I'm seeing different results:

$ gh pr status --repo=coder/coder --json headRefName,number{"createdBy":[{"headRefName":"dk/fix-preview","number":13404},{"headRefName":"dk/verify-agent","number":13280}],"needsReview":[]}

I think the script should just accept a PR as input. WDYT?

@matifali
Copy link
Member

@dannykopping I think as the branch is actually not on thecoder/coder repo, that's why we are not able to get it. Skipping--repo=coder/coder seems to work fine along with.currentBranch

@dannykopping
Copy link
ContributorAuthor

@dannykopping I think as the branch is actually not on thecoder/coder repo, that's why we are not able to get it. Skipping--repo=coder/coder seems to work fine along with.currentBranch

Not so for me:

$ gh pr status --json headRefName,number --jq'.currentBranch'| cat
matifali reacted with eyes emoji

@dannykopping
Copy link
ContributorAuthor

pr list is reliable, and works irrespective of forks.

$ ./scripts/deploy-pr.sh -d -n -y                                                                          dry runbranchName: dk/verify-agentprNumber: 13280experiments: build:falsedeploy:true$ git co dk/fix-preview Switched to branch'dk/fix-preview'$ ./scripts/deploy-pr.sh -d -n -ydry runbranchName: dk/fix-previewprNumber: 13404experiments: build:falsedeploy:true
matifali reacted with thumbs up emoji

Signed-off-by: Danny Kopping <danny@coder.com>
@matifali
Copy link
Member

@dannykopping OK lets usepr list push your changes, and I can try to run it from coder/coder

@dannykopping
Copy link
ContributorAuthor

@dannykopping OK lets usepr list push your changes, and I can try to run it from coder/coder

$ ./scripts/deploy-pr.sh -d                                                                              Are you sure you want to deploy? (y/n) ybranchName: dk/fix-previewprNumber: 13404experiments: build:falsedeploy:truecould not create workflow dispatch event: HTTP 422: No ref found for: dk/fix-preview (https://api.github.com/repos/coder/coder/actions/workflows/60960476/dispatches)

Same issue as before (not that I expected this to be fixed with this change, but worth noting).

matifali reacted with eyes emoji

@matifali
Copy link
Member

matifali commentedMay 30, 2024
edited
Loading

It works fine now in dry run. Can you try deploying from the fork for real? So that we can test if all secrets are accessible, too.

@matifali
Copy link
Member

@dannykopping OK lets usepr list push your changes, and I can try to run it from coder/coder

$ ./scripts/deploy-pr.sh -d                                                                              Are you sure you want to deploy? (y/n) ybranchName: dk/fix-previewprNumber: 13404experiments: build:falsedeploy:truecould not create workflow dispatch event: HTTP 422: No ref found for: dk/fix-preview (https://api.github.com/repos/coder/coder/actions/workflows/60960476/dispatches)

Same issue as before (not that I expected this to be fixed with this change, but worth noting).

Same issue for me. Can not deploy.

@matifali
Copy link
Member

I think we need to allow running this workflow on forks in repo settings.

@dannykopping
Copy link
ContributorAuthor

@dannykopping OK lets usepr list push your changes, and I can try to run it from coder/coder

$ ./scripts/deploy-pr.sh -d                                                                              Are you sure you want to deploy? (y/n) ybranchName: dk/fix-previewprNumber: 13404experiments: build:falsedeploy:truecould not create workflow dispatch event: HTTP 422: No ref found for: dk/fix-preview (https://api.github.com/repos/coder/coder/actions/workflows/60960476/dispatches)

Same issue as before (not that I expected this to be fixed with this change, but worth noting).

Same issue for me. Can not deploy.

I think this is actually a good thing.

gh workflow run in the deploy script is trying to reference the workflow script in a branch which is outside the main repo. I don't think we should enable this.

I think we'll have to close this PR.

If the my branch from#13280 was not present incoder/coder (like I was expecting), then I would've gotten the422 as above. So this whole PR was fruit of the poisonous tree.

matifali reacted with laugh emoji

@matifali
Copy link
Member

gh workflow run in the deploy script is trying to reference the workflow script in a branch which is outside the main repo. I don't think we should enable this.

Yes we do have the protection to check for membership but that check is part of the same workflow so not safe at all.

@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 30, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@matifalimatifalimatifali approved these changes

Assignees

@dannykoppingdannykopping

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@dannykopping@matifali

[8]ページ先頭

©2009-2025 Movatter.jp