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

Auth changes for vNext #987

Closed
wants to merge 20 commits into from
Closed

Conversation

kfcampbell
Copy link
Member

@kfcampbell kfcampbell commented Nov 26, 2021

Part of #980.

TODO:

  • Can we do a lookup and obviate the need for GITHUB_TEST_ORGANIZATION?
  • Ensure skipUnlessMode does the right thing with organization, user, and anonymous tests
    • I don't think this is the case now since I'm seeing a bunch of requests to GET /orgs/kfcampbell HTTP/1.1 in the tests
  • Perform same array of testing matrix as I did with the main branch earlier

This WIP PR aims to:

  • Deprecate GITHUB_ORGANIZATION environment variable (and organization provider block configuration) in favor of GITHUB_OWNER
  • If no token is present, still process owner variables so that unauthenticated requests may be made against public org-owned repos
  • Improve error handling and messaging around missing tokens/owner configuration (currently it's pretty difficult to figure out this is a problem and act appropriately
  • Tweaks to avoid pointless requests (for example, when no token or owner is configured, we try a pointless request to e.g. api.github.com//example-repo, which obviously won't
  • Docs
    • Clarify how provider authentication works in the case where both the integrations and hashicorp providers are used (I keep returning to @tibbes's excellent comment here about it)
    • Specify in our docs exactly how the decision tree works for each form of authentication (app, provider PAT config, environment variable PAT config) and when/how they can be combined
    • Return an error in some cases where our requests fail but the provider doesn't return an error code

Note that there is another small behavior change: I doubt anyone is using this, and I don't think it's particularly good practice, but I believe it used to be possible to run actions on behalf of both an organization and an individual account, using the same token, in the same terraform file. After this v5.0.0 change is released, this will no longer be true, and setting different owners for a token will require different provider configuration. This change is intentional and (I think) a good thing.

// TODO(kfcampbell): this is a problem. how are we going to know it's an organization
// in acceptance tests?
// ideas:
// - we could perform a lookup online to determine the type
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this option most given we cache and reuse the result as aggressively as possible. Debug logs suggest we currently resolve an owner to an organization multiple times.

@jcogilvie
Copy link

Will this address #977?

@kfcampbell kfcampbell added the v5 label Jan 18, 2022
@kfcampbell kfcampbell removed the v5 label Oct 31, 2022
@nickfloyd nickfloyd added the Status: Stale Used by stalebot to clean house label Nov 30, 2022
@nickfloyd
Copy link
Contributor

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot removed the Status: Stale Used by stalebot to clean house label Dec 1, 2022
@arledesma
Copy link

@kfcampbell if this is still valid then can you update the title to reference vNEXT to align with your new major version strategy? I understand that the branch name would look awkward but the title is misleading as there is already a v5.

@posquit0
Copy link

I hope that this feature will be available in 2023. 🥲

@kfcampbell kfcampbell changed the title Auth changes for v5 Auth changes for vNext Feb 3, 2023
@kfcampbell
Copy link
Member Author

if this is still valid then can you update the title to reference vNEXT to align with your new major version strategy?

Done!

I hope that this feature will be available in 2023. 🥲

I apologize for the slow rate of our feature development here. If you'd like to take this branch and run with it, please feel free!

Copy link

github-actions bot commented Nov 1, 2023

👋 Hey Friends, this pull request has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Nov 1, 2023
@github-actions github-actions bot closed this Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Used by stalebot to clean house
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants