Codecov Report

Merging#1296 (cab343c) intomaster (9d6c188) willnot change coverage.
The diff coverage isn/a.

@@           Coverage Diff           @@##           master    #1296   +/-   ##=======================================  Coverage   78.26%   78.26%           =======================================  Files          12       12             Lines        2894     2894           =======================================  Hits         2265     2265             Misses        629      629

Flags with carried forward coverage won't be shown.Click here to find out more.

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update9d6c188...cab343c. Read thecomment docs.

We found this PR after we tried including gitlab-cli in a GitLab CI/CD pipeline job. This PR looks like it will achieve what we wanted to do (drive the GitLab API without creating a config file in each job) 🎉

One outstanding wishlist item we had was if the tool could look for the same environment variables that are set inside a GitLab CI/CD job, i.e. CI_JOB_TOKEN, CI_SERVER_URL and so on1. If the tool shared these varaibles, it could authenticate with the GitLab instance without any extra code written in CI/CD jobs - is that something that has already been considered?

We found this PR after we tried including gitlab-cli in a GitLab CI/CD pipeline job. This PR looks like it will achieve what we wanted to do (drive the GitLab API without creating a config file in each job) 🎉

One outstanding wishlist item we had was if the tool could look for the same environment variables that are set inside a GitLab CI/CD job, i.e. CI_JOB_TOKEN, CI_SERVER_URL and so on1. If the tool shared these varaibles, it could authenticate with the GitLab instance without any extra code written in CI/CD jobs - is that something that has already been considered?

Yes, part of this was meant to implement zero-config for CI jobs. But I need to maybe tweak and add tests to really check all scenarios (for some endpoints, a PAT is needed, so we should allow overriding in a sensible precedence between cli args/env/config files/CI env). Currently the precedence planned is:

I'll try to get back to this PR in the coming days :)

Yes, part of this was meant to implement zero-config for CI jobs. But I need to maybe tweak and add tests to really check all scenarios (for some endpoints, a PAT is needed, so we should allow overriding in a sensible precedence between cli args/env/config files/CI env). Currently the precedence planned is:

python-gitlab/gitlab/__init__.py

Lines 224 to 232 incab343c

	1. Explicitly provided CLI arguments,
	2. Environment variables,
	3. Configuration files:
	a. explicitly defined config files:
	i. via the `--config-file` CLI argument,
	ii. via the `PYTHON_GITLAB_CFG` environment variable,
	b. user-specific config file,
	c. system-level config file,
	4. Environment variables always present in CI (CI_SERVER_URL, CI_JOB_TOKEN).

Aha. I'm blind 👀

That does indeed look like exactly the behaviour we wanted.

Codecov Report

Merging#1296 (13467c5) intomaster (ce4bc0d) willincrease coverage by0.03%.
The diff coverage is100.00%.

@@            Coverage Diff             @@##           master    #1296      +/-   ##==========================================+ Coverage   91.57%   91.61%   +0.03%==========================================  Files          74       74                Lines        4263     4281      +18     ==========================================+ Hits         3904     3922      +18  Misses        359      359

Flags with carried forward coverage won't be shown.Click here to find out more.

What do you think about usingargparse.ArgumentParser..add_mutually_exclusive_group() ?

For example:

private_token_group = parser.add_mutually_exclusive_group()private_token_group.add_argument(        "--private-token",        help=("GitLab private token"),        required=False,    )private_token_group.add_argument(        "--private-token-env-var",        help=("Name of environment variable containing GitLab private token"),        required=False,    )

But now that I look at the code I'm not sure where to do the processing to load the environment variable 😟

I'll just leave this comment anyway and maybe it gives you an idea 🤷‍♂️

.add_mutually_exclusive_group()

Thanks! Yes, I think it might be useful (not for arg/var exclusive I think but maybe for different types of tokens). But yes sometimes we want to validate those a bit later, because things like CI_JOB_TOKEN might always be present 🤦

I think we just need to write a bunch of tests for expected behaviors in all the possible combinations, and write the code based on that.

@JohnVillalovos took almost a year to get back to this, but ready for another look 😅