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 config of registry repo URLs with ENV #208

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

TransGirlCodes
Copy link
Contributor

@TransGirlCodes TransGirlCodes commented Jul 23, 2019

This PR adds the ability to get the port from ENV for the WebUI.
It also addresses #74, by allowing URLs for registries to be specified through ENV.

This is useful for services like Heroku where the URL used for the repository contains the access token: https://USER:[email protected]/User/Repo.

I'm not sure, but currently, I don't think the token field in config.commentbot.toml is actually used.
Because providing an access token for this field, and then using a normal https URL for the registries results in authentication errors for me, whereas using repo urls of the form https://USER:[email protected]/User/Repo works fine.

EDIT: I stand corrected, the token field is used, in parts of the comment bot where GitHub.jl is used, it's just seemingly not used when it comes to cloning the registry repo and doing the stuff Regedit/Registration service does.

@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #208 into master will decrease coverage by 6.69%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #208     +/-   ##
========================================
- Coverage   13.21%   6.51%   -6.7%     
========================================
  Files          27      27             
  Lines        1256    1258      +2     
========================================
- Hits          166      82     -84     
- Misses       1090    1176     +86
Impacted Files Coverage Δ
src/webui/WebUI.jl 1.47% <0%> (-35.15%) ⬇️
src/commentbot/CommentBot.jl 0.86% <0%> (-0.04%) ⬇️
src/webui/routes/select.jl 0% <0%> (-100%) ⬇️
src/webui/providers.jl 0% <0%> (-100%) ⬇️
src/webui/routes/index.jl 0% <0%> (-100%) ⬇️
src/webui/webutils.jl 7.14% <0%> (-71.43%) ⬇️
src/webui/routes/register.jl 0% <0%> (-47.23%) ⬇️
src/webui/gitutils.jl 0.75% <0%> (-10.79%) ⬇️
src/management.jl 0% <0%> (-10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d9f684...d774a99. Read the comment docs.

@TransGirlCodes TransGirlCodes changed the title ENV Variable configuration of comment bot for Heroku Allow config of registry URLs with ENV Jul 24, 2019
@TransGirlCodes TransGirlCodes changed the title Allow config of registry URLs with ENV Allow config of registry repo URLs with ENV Jul 24, 2019
@TransGirlCodes
Copy link
Contributor Author

TransGirlCodes commented Jul 24, 2019

There is an alternative to this, which is to do something like this in the Docker:

RUN git config --global url.”https://{token}:@github.com/".insteadOf “https://github.com/"

If you'd rather that I try that and see if that works before changing Registrator code, that's cool, I can try that this week and report back.

# assign it a default one, obtained from ENV.
if !haskey(target_registry, "repo")
specific_env_key = string(uppercase(target_registry_name), "_REGISTRY_URL")
envkey = ifelse(haskey(ENV, specific_env_key), specific_env_key, "DEFAULT_REGISTRY_URL")
Copy link
Member

Choose a reason for hiding this comment

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

This and the next line can be simplified to:

target_registry["repo"] = get(ENV, specific_env_key, ENV["DEFAULT_REGISTRY_URL"])

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