Skip to content

Conversation

@nicholsn
Copy link

This pull request introduces support for using an environment variable (CODEARTIFACT_AUTH_TOKEN) to bypass AWS API calls when interacting with CodeArtifact, enhancing usability in CI/CD environments and simplifying token management. The changes include updates to documentation, backend logic, and test cases.

Documentation Updates:

  • README.md: Added instructions for setting the CODEARTIFACT_AUTH_TOKEN environment variable to bypass AWS API calls, with examples for usage in CI/CD environments.

Backend Logic Enhancements:

  • keyrings/codeartifact.py: Updated the get_password method to check for the CODEARTIFACT_AUTH_TOKEN environment variable and use it if available, logging the usage for transparency.
  • keyrings/codeartifact.py: Imported the os module to support environment variable retrieval.

Testing Improvements:

  • tests/test_backend.py: Added a new test case (test_get_credential_from_env) to verify that the backend can retrieve credentials from the CODEARTIFACT_AUTH_TOKEN environment variable and bypass client creation when the variable is set.

@nicholsn
Copy link
Author

@jmkeyes snagging this from alxbrd@6f50c45

@nicholsn
Copy link
Author

Note that I'm using this in a GHA CI/CD pipeline where I'm installing dependencies from codeartifact in a docker container that is being pushed to ECR in a different account. This is somewhat similar use case as #6 where I assume a role to get the token outside of docker and then pass it in as an env var to install the package.

@nicholsn
Copy link
Author

nicholsn commented Aug 6, 2025

@jmkeyes any thoughts on this?

@jmkeyes
Copy link
Owner

jmkeyes commented Aug 12, 2025

Hey @nicholsn! Thanks for the contribution!

I think this is a great idea: being able to short circuit the keyring backend lookup process and pass through the token with an environment variable is a great idea.

I do have some outstanding questions:

  • Did you get permission from @alxbrd to use this patch? It's a small patch derived from another, but I want to preserve the original author(s) involved in making the patch.
  • The version @alxbrd created looks up the environment variable specified in the keyringrc.cfg configuration file, rather than hard-coding it as a constant. Do you find value in that approach?
  • Did you use an LLM (ChatGPT, Claude, Copilot, Gemini, etc) to prepare this PR or the code submitted in it? I want to attribute ownership to the original author/creator and the output from LLMs make/doing that a bit tougher.

I'll submit my review of the code itself shortly.

)

# Check for token in environment variable
token_from_env = os.getenv("CODEARTIFACT_AUTH_TOKEN")
Copy link
Owner

Choose a reason for hiding this comment

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

This block could be moved to the top of the get_password method and shortcut most of the logic.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, moved it up!

)

# Check for token in environment variable
token_from_env = os.getenv("CODEARTIFACT_AUTH_TOKEN")
Copy link
Owner

Choose a reason for hiding this comment

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

The CODEARTIFACT_AUTH_TOKEN environment variable could be moved into a constant.

Copy link
Author

Choose a reason for hiding this comment

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

Done, and brought in pulling the name of the environment variable to use from the config

@alxbrd
Copy link

alxbrd commented Aug 13, 2025

Hey @nicholsn! Thanks for the contribution!

I think this is a great idea: being able to short circuit the keyring backend lookup process and pass through the token with an environment variable is a great idea.

I do have some outstanding questions:

* Did you get permission from @alxbrd to use this patch? It's a small patch derived from another, but I want to preserve the original author(s) involved in making the patch.

* The version @alxbrd created looks up the environment variable specified in the `keyringrc.cfg` configuration file, rather than hard-coding it as a constant. Do you find value in that approach?

* Did you use an LLM (ChatGPT, Claude, Copilot, Gemini, etc) to prepare this PR or the code submitted in it? I want to attribute ownership to the original author/creator and the output from LLMs make/doing that a bit tougher.

I'll submit my review of the code itself shortly.

@jmkeyes Please find my answers to the questions you raised:

  1. Yes. Absolutely fine for this PR to be merged upstream.
  2. Would it be possible to point me to the lines you refer to in this question?
  3. No.

@jmkeyes
Copy link
Owner

jmkeyes commented Aug 13, 2025

  1. Yes. Absolutely fine for this PR to be merged upstream.

That works for me!

  1. Would it be possible to point me to the lines you refer to in this question?

I was referring to your commit 6f50c45 that @nicholsn referenced above which uses config.get('env_variable', 'CODEARTIFACT_AUTH_TOKEN').

  1. No.

Ok, perfect!

@nicholsn
Copy link
Author

@jmkeyes just pushed a few commits to hopefully address the remaining questions.

@alxbrd thanks for chiming in while I was away!

@nicholsn
Copy link
Author

@jmkeyes lmk if you want to see any additional changes before merging

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