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

Allow passed authentication params on init #41

Merged

Conversation

kaosx5s
Copy link
Collaborator

@kaosx5s kaosx5s commented May 16, 2023

Description

Thanks for contributing this Pull Request. Make sure that you send in this Pull Request to the master branch of this repository, add a brief description, and tag the relevant issue(s) and PR(s) below.

  • Relevant Issues :

  • What Changed and Why? :

    • Adds support to pass a git token
    • Adds support to pass a git username and git password
  • Type of change :

    • New feature
    • Bug fix for existing feature
    • Code quality improvement
    • Addition or Improvement of tests
    • Addition or Improvement of documentation
  • Other:

    • Add unit tests
    • Add documentation

@kaosx5s kaosx5s force-pushed the allow-passed-git-token-init branch from 343ad4c to a099d6a Compare May 16, 2023 21:24
@kaosx5s kaosx5s mentioned this pull request May 16, 2023
7 tasks
README.md Outdated Show resolved Hide resolved
AgarFu
AgarFu previously requested changes May 17, 2023
Copy link
Collaborator

@AgarFu AgarFu left a comment

Choose a reason for hiding this comment

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

let's be consistent with the options we provide, if we want to allow passing credentials via constructor parameters, let's allow all of them.

Then we can do something like:

def __ init__(self, username=None, password=None, token=None):
  username = os.environment.get('GITHUB_USERNAME', username)
  password = os.environment.get('GITHUB_PASSWORD', password)
  token = os.environment.get('GITHUB_TOKEN', token)
  ...
  # Check for credentials
  # Instantiate the client

gordian/repo.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Signed-off-by: kaosx5s <>
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #41 (f650965) into master (5d59323) will increase coverage by 3.49%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   77.64%   81.13%   +3.49%     
==========================================
  Files          11       11              
  Lines         492      493       +1     
  Branches       73       77       +4     
==========================================
+ Hits          382      400      +18     
+ Misses         96       76      -20     
- Partials       14       17       +3     
Files Coverage Δ
gordian/repo.py 78.36% <100.00%> (+10.12%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kaosx5s kaosx5s changed the title Allow passed git token on init Allow passed authentication params on init May 17, 2023
Copy link
Collaborator

@coreycaverly coreycaverly left a comment

Choose a reason for hiding this comment

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

LGTM

@coreycaverly
Copy link
Collaborator

@AgarFu need you to approve changes.

@coreycaverly coreycaverly dismissed AgarFu’s stale review October 4, 2023 21:43

This was addressed by @kaox5s

@coreycaverly coreycaverly merged commit e9f1eb1 into argoproj-labs:master Oct 4, 2023
6 checks passed
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.

4 participants