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

Improvement: Update Permissions of generated workflows to support Private Repos #7691

Closed
wants to merge 5 commits into from

Conversation

Whats-A-MattR
Copy link

Minor update / patch to include support for Private Repositories out of the box.

Due to id-token: write being set statically on the generated workflow, other permissions are set / unset to none as outlined in GitHub Action docs.

If you specify the access for any of these scopes, all of those that are not specified are set to none.
Source: Github Actions Documentation

I have updated the permissions constant to include
contents: read #Required to enable use with Private Repositories
The Action should work out of the box now regardless of whether the Repository is Public or Private.

@Whats-A-MattR
Copy link
Author

Whats-A-MattR commented Jun 12, 2024

Not sure if this has been looked at yet, would be great to get this implemented though.
Very minor change that reduces friction when deploying AZ Function Apps via GitHub Action generated in the AZ Function Portal, specifically when using Private Repos.
@btardif @shimedh @andimarc @jvano

@Whats-A-MattR
Copy link
Author

Any response at all would be great, even if it's a no... I see other PRs getting closed while this sits idle, for a rather small change.

@spellegrino021
Copy link
Contributor

We are looking at it! Will update based on what we decide to do.

@Whats-A-MattR
Copy link
Author

We are looking at it! Will update based on what we decide to do.

sounds good, thank you

@Whats-A-MattR
Copy link
Author

@spellegrino021 it is now August :)

@spellegrino021
Copy link
Contributor

Hey! Testing this out now. For Web Apps I tested with private repositories and was able to have a successful build and deployment. I think you were specifically having an issue with Function apps but I wasn't able to repro with a python function app. Mind telling me which stack you were working with and the exact error and step of the workflow file it was happening at? The change you are proposing is for all web and function apps, so it might be better to limit it to just function apps as needed.

@Whats-A-MattR
Copy link
Author

The stack was using a Function App with PowerShell Core, deploying from GitHub Actions with the premade/canned workflow.
It's a pretty minor update, but was required to make publishing work when I used it. It only grants read access, which is assumed when you set up a repo for use in CI/CD.
As revealed in the GitHub Actions docs, permissions are explicit so you must include any required permission if setting any.

@Whats-A-MattR
Copy link
Author

Are we any closer to a merge or close?

Copy link
Author

@Whats-A-MattR Whats-A-MattR left a comment

Choose a reason for hiding this comment

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

Resolved conflicts in updating from base.

@Whats-A-MattR
Copy link
Author

Any particular this very small improvement was rejected @spellegrino021 ?
I've added a single line, in accordance with GitHub Actions documentation, and I've included said documentation in my PR.

What is the issue here?

@mwasson74
Copy link

mwasson74 commented Dec 2, 2024

Following along as I ran into this issue, too

Edit: Confirmed adding contents: read to permissions: worked

@Whats-A-MattR
Copy link
Author

Following along as I ran into this issue, too

Edit: Confirmed adding contents: read to permissions: worked

Sadly @spellegrino021 closed the PR for some undisclosed reason, so we'll continue resorting to manually updating our workflows on new deploys because adding one line which is documented as required by GitHub docs is too much effort? Even though they just needed to read a paragraph of GitHub docs and let a test suite run?
Hopefully anyone else facing the same issue stumbles upon this so they can resolve it manually.

@spellegrino021
Copy link
Contributor

I ended up including this in a different PR which will ship in Feb.

@mwasson74
Copy link

@spellegrino021 May we know which PR so we can subscribe to it?

@Whats-A-MattR
Copy link
Author

I ended up including this in a different PR which will ship in Feb.

Very cool, close my PR and have the change I identified and created a PR for copied into a different PR.
I know the change is only a few lines, but the work that went into finding everything was non-trivial. Receiving the contribution would have been nice.

Thanks 👍🏻

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