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

feat: Add token auth to GitLab/GitHub hosting providers #1761

Merged
merged 24 commits into from Dec 16, 2022
Merged

feat: Add token auth to GitLab/GitHub hosting providers #1761

merged 24 commits into from Dec 16, 2022

Conversation

tdido
Copy link
Member

@tdido tdido commented Jul 8, 2022

Description

This PR adds an optional "token" argument the ability to get a token from environment variables to the gitlab() and github() code hosting provider. This allows for the usage of modules stored in private repositories.

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Closes #1301

@tdido tdido changed the title Add token authentication to GitLab hosting provider [feat] Add token authentication to GitLab hosting provider Jul 8, 2022
@tdido tdido changed the title [feat] Add token authentication to GitLab hosting provider feat: Add token authentication to GitLab hosting provider Jul 8, 2022
@tdido tdido changed the title feat: Add token authentication to GitLab hosting provider feat: Add token auth to GitLab hosting provider Jul 8, 2022
@tdido tdido changed the title feat: Add token auth to GitLab hosting provider feat: Add token auth to GitLab/GitHub hosting providers Jul 8, 2022
@tdido tdido marked this pull request as draft July 18, 2022 14:11
@tdido
Copy link
Member Author

tdido commented Jul 18, 2022

Converting to draft as during some testing I realised the "conda_env" path doesn't contain the token e.g. when using a module in a private repo that contains a conda environment. Looking into it now.

@johanneskoester any comments would be appreciated.

@tdido
Copy link
Member Author

tdido commented Jul 18, 2022

Example error message:

Not a conda environment: /my/workflow/.test/https:/gitlab.com/api/v4/projects/myproject/repository/files/workflow%2Fenvs%2Fenvironment.yaml/raw?ref=main

So it's treating the environment URL as local. I guess the module import requires redefining the env in this case?

Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Nice improvement! For the sake of code coverage, may I ask you to extend the corresponding testcase to also check the token parameter? You can just use the environment variable GITHUB_TOKEN for this, and simply still point to a public file.

tdido and others added 2 commits July 19, 2022 12:36
Co-authored-by: Johannes Köster <johannes.koester@uni-due.de>
@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tdido
Copy link
Member Author

tdido commented Jul 29, 2022

@johanneskoester seems like conda env files are still fetched without the token. I've tested hardcoding it in the conda deployment file [1] and it works. Any pointers on the best way to make the token available there?

[1] https://github.com/tdido/snakemake/blob/435913d63cc42ac7b9e0cf438673e339aa25d055/snakemake/deployment/conda.py#L843

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tdido tdido marked this pull request as ready for review October 20, 2022 14:04
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tdido
Copy link
Member Author

tdido commented Oct 20, 2022

Hey @johanneskoester. I've had to change the approach since I couldn't find a way to get environment files to use the token with the previous approach. It now would seem like all cases are covered, although it implies adding a command line argument for the hosting provider token. Please let me know what you think.

@johanneskoester
Copy link
Contributor

A cli arg for the token is not an option IMHO, because then the token will be inside of the bash history, which is a security risk. Can you explain what exactly fails when just fetching the token from os.environ?

@tdido
Copy link
Member Author

tdido commented Dec 13, 2022

Right, yes, that makes sense. The failure I was referring to was the one when adding the argument to the gitlab() function, where it doesn't pass the token to the conda environments it fetches.

I'll change the cli arg to os.environ instead, should be straightforward.

@tdido
Copy link
Member Author

tdido commented Dec 15, 2022

@johanneskoester I've switched to using an env variable for the token and it seems to be working fine. Please let me know whether the implementation looks OK.

Copy link
Member Author

@tdido tdido left a comment

Choose a reason for hiding this comment

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

Some requested changes not needed anymore with env vars approach, not sure how to mark as solved...

snakemake/workflow.py Outdated Show resolved Hide resolved
snakemake/deployment/conda.py Outdated Show resolved Hide resolved
@johanneskoester
Copy link
Contributor

Can you please run black on your changes?

Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Almost forgot: could you also add a sentence about the use of the two env vars to the respective section in the docs? (this one)

@sonarcloud
Copy link

sonarcloud bot commented Dec 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johanneskoester johanneskoester merged commit e03a3b4 into snakemake:main Dec 16, 2022
johanneskoester pushed a commit that referenced this pull request Jan 18, 2023
🤖 I have created a release *beep* *boop*
---


##
[7.20.0](v7.19.1...v7.20.0)
(2023-01-18)


### Features

* add tes token
([#1966](#1966))
([59a8fa0](59a8fa0))
* Add token auth to GitLab/GitHub hosting providers
([#1761](#1761))
([e03a3b4](e03a3b4)),
closes [#1301](#1301)
* allow for human friendly resource definitions (e.g. mem="5GB",
runtime="1d")
([#1861](#1861))
([24610ac](24610ac))


### Bug Fixes

* 🐛 - fix hyperlink
([#2046](#2046))
([9519d31](9519d31))
* Catch missing error stream in Slurm executor
([#2063](#2063))
([c21fc7e](c21fc7e))
* correctly parse empty values in config cli
([#2032](#2032))
([1b0689d](1b0689d))
* Correctly parse UserDicts in executors
([#2016](#2016))
([e3926fa](e3926fa))
* Fix handling of --jobs in no-exec state
([#2029](#2029))
([e8e8222](e8e8222))
* make `--show-failed-logs` handle empty log files
([#2039](#2039))
([683c6f2](683c6f2)),
closes [#2023](#2023)
* make python version check more robust
([#2058](#2058))
([e685621](e685621))
* parsing error when last line is comment
([#2054](#2054))
([a928dd4](a928dd4))
* prevent overriding of retries when set to 0
([#2053](#2053))
([a328f3e](a328f3e))
* propagate attempt count from group to subjobs
([#2052](#2052))
([da3f1c0](da3f1c0))
* remove overflow from rulegraph div in report
([9a0aaa7](9a0aaa7))
* skip type checks of missing dir in touch mode
([#2051](#2051))
([ae00c25](ae00c25))
* slurm default_resources quoting
([#2043](#2043))
([47d3fc3](47d3fc3))
* Update list of python versions in classifiers
([#2020](#2020))
([7a98100](7a98100))
* use short argument name for `--chdir` for compatibility with Slurm
&lt;=v17 ([#2040](#2040))
([a9ed3ec](a9ed3ec))


### Documentation

* Fix typo in SLURM help text
([#2049](#2049))
([79b7025](79b7025))
* mention XDG_CACHE_HOME
([#2057](#2057))
([ec2ef45](ec2ef45))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
jakevc pushed a commit to jakevc/snakemake that referenced this pull request Feb 7, 2023
🤖 I have created a release *beep* *boop*
---


##
[7.20.0](snakemake/snakemake@v7.19.1...v7.20.0)
(2023-01-18)


### Features

* add tes token
([snakemake#1966](snakemake#1966))
([59a8fa0](snakemake@59a8fa0))
* Add token auth to GitLab/GitHub hosting providers
([snakemake#1761](snakemake#1761))
([e03a3b4](snakemake@e03a3b4)),
closes [snakemake#1301](snakemake#1301)
* allow for human friendly resource definitions (e.g. mem="5GB",
runtime="1d")
([snakemake#1861](snakemake#1861))
([24610ac](snakemake@24610ac))


### Bug Fixes

* 🐛 - fix hyperlink
([snakemake#2046](snakemake#2046))
([9519d31](snakemake@9519d31))
* Catch missing error stream in Slurm executor
([snakemake#2063](snakemake#2063))
([c21fc7e](snakemake@c21fc7e))
* correctly parse empty values in config cli
([snakemake#2032](snakemake#2032))
([1b0689d](snakemake@1b0689d))
* Correctly parse UserDicts in executors
([snakemake#2016](snakemake#2016))
([e3926fa](snakemake@e3926fa))
* Fix handling of --jobs in no-exec state
([snakemake#2029](snakemake#2029))
([e8e8222](snakemake@e8e8222))
* make `--show-failed-logs` handle empty log files
([snakemake#2039](snakemake#2039))
([683c6f2](snakemake@683c6f2)),
closes [snakemake#2023](snakemake#2023)
* make python version check more robust
([snakemake#2058](snakemake#2058))
([e685621](snakemake@e685621))
* parsing error when last line is comment
([snakemake#2054](snakemake#2054))
([a928dd4](snakemake@a928dd4))
* prevent overriding of retries when set to 0
([snakemake#2053](snakemake#2053))
([a328f3e](snakemake@a328f3e))
* propagate attempt count from group to subjobs
([snakemake#2052](snakemake#2052))
([da3f1c0](snakemake@da3f1c0))
* remove overflow from rulegraph div in report
([9a0aaa7](snakemake@9a0aaa7))
* skip type checks of missing dir in touch mode
([snakemake#2051](snakemake#2051))
([ae00c25](snakemake@ae00c25))
* slurm default_resources quoting
([snakemake#2043](snakemake#2043))
([47d3fc3](snakemake@47d3fc3))
* Update list of python versions in classifiers
([snakemake#2020](snakemake#2020))
([7a98100](snakemake@7a98100))
* use short argument name for `--chdir` for compatibility with Slurm
&lt;=v17 ([snakemake#2040](snakemake#2040))
([a9ed3ec](snakemake@a9ed3ec))


### Documentation

* Fix typo in SLURM help text
([snakemake#2049](snakemake#2049))
([79b7025](snakemake@79b7025))
* mention XDG_CACHE_HOME
([snakemake#2057](snakemake#2057))
([ec2ef45](snakemake@ec2ef45))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Use modules from private github repository
2 participants