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

Add env var as credential input in cli #1456

Closed
wants to merge 1 commit into from

Conversation

AvengerMoJo
Copy link

#1453

Taking env var as consideration when the osc is being called the first time.
This allows automation in the workflow without depending on an interactive session.

The implementation is simply checking if OSC_USERNAME, OSC_PASSWORD or OSC_CREDENTIAL
is present and prioritized over the interactive input.
README is also updated accordingly.

osc/conf.py Outdated Show resolved Hide resolved
osc/conf.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Dec 6, 2023

Hello @AvengerMoJo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-12-11 06:12:36 UTC

@AvengerMoJo AvengerMoJo force-pushed the wip_env_auth_cli branch 2 times, most recently from f1196aa to 04cf3f5 Compare December 11, 2023 06:12
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (608fc76) 31.05% compared to head (778def9) 31.03%.
Report is 6 commits behind head on master.

Files Patch % Lines
osc/conf.py 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1456      +/-   ##
==========================================
- Coverage   31.05%   31.03%   -0.02%     
==========================================
  Files          49       49              
  Lines       17758    17768      +10     
==========================================
  Hits         5514     5514              
- Misses      12244    12254      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Alex Lau (AvengerMoJo) <alau@suse.com>
@dmach
Copy link
Contributor

dmach commented Jan 4, 2024

@AvengerMoJo I finally got to this pull request and I really like the idea of setting credentials through the env vars.
Your patch would work just fine in the containers, but then I realized that we could extend this so you could use the variables to override osc settings at any time. It's also in sync with the current implementation of osc.conf.get_config() that allows you overriding settings via OSC_APIURL=..., OSC_BUILD_JOBS=... etc.
Hmm, I should mention that in the docs. It's mentioned in the library docs, but that's not enough.

I ended up implementing this: #1463.
I'm sorry I did not post any feedback to this PR earlier, but after some prototyping I realized I'm writing something completely else that is competing with your PR rather than building on top of it and I decided to finish it and ask for your opinion.
I also had to fix some issues I caused in the previous commits first: #1462

Could you test if #1463 works for you?
Please note that setting OSC_CONFIG= is essential to avoid creating oscrc.

@AvengerMoJo
Copy link
Author

I am pulling your branch to build a customized docker image to test it against src.o.o in gitea and let you know soon .

@AvengerMoJo
Copy link
Author

#1463 should replace this PR and the function is working in the cli using OSC_CONFIG= , test with gitea action container with the osc patch https://src.opensuse.org/alexlau/bash-custom-action/actions/runs/9

@AvengerMoJo AvengerMoJo closed this Jan 8, 2024
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.

5 participants