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

Feature: Gitlab module will not always work with internal service names #9537

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

xrow
Copy link

@xrow xrow commented Jan 6, 2025

SUMMARY

Enabling this switch allows the the gitlab module to communicate over internal host names with the api.

gitlab.example.com <- Public Host / Ingress
gitlab.gitlab.svc:8080 <- Internal Service name in Kubernetes

ISSUE TYPE
  • Feature Pull Request
  • Refactoring Pull Request
COMPONENT NAME

gitlab

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request gitlab module_utils module_utils new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Jan 6, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Jan 6, 2025
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Can you please add a changelog fragment? Thanks.

Also, did you check which versions of the Python gitlab library support keep_base_url?

@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Jan 7, 2025
@xrow
Copy link
Author

xrow commented Jan 7, 2025

@felixfontein keep_base_url was added in https://github.com/python-gitlab/python-gitlab/releases/tag/v3.8.0 which is 2 years ago. Do I need to add a contraint somewhere? Else feel free to merge.

@felixfontein
Copy link
Collaborator

The modules likely also support older versions of python-gitlab, and this PR would break these older versions. (gitlab_branch for example explicitly supports python-gitlab >= 2.3.0.) Can you adjust the PR to only provide the option for versions that support it?

@xrow
Copy link
Author

xrow commented Jan 7, 2025

@felixfontein Hi, I am a bit confused. The change affects all the modules, since it is a change in the client used globally. To me it looks more like we shall lift the requirements to >= 3.8.0 for all gitlab modules accept gitlab-runner.

@felixfontein
Copy link
Collaborator

To me it looks more like we shall lift the requirements to >= 3.8.0 for all gitlab modules accept gitlab-runner.

That's a breaking change and will not get merged.

@xrow
Copy link
Author

xrow commented Jan 7, 2025

Ok that is fine with me... though can you plan it for next major?

@felixfontein
Copy link
Collaborator

A breaking change first requires a deprecation, which doesn't exist in this case. Also I don't see why the minimum version should be bumped if this can be easily solved by checking the version of python-gitlab. The file you changed has multiple examples of how that works.

@xrow
Copy link
Author

xrow commented Jan 7, 2025

@felixfontein Good point. I see now what you want.

@xrow
Copy link
Author

xrow commented Jan 8, 2025

@felixfontein You can review again. Thanks for your tip.

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 8, 2025
@xrow
Copy link
Author

xrow commented Jan 8, 2025

@felixfontein "FATAL: Environment --docker fedora40 is unknown. " Looks like there are temporary runner issues. Nothing i can fix here.

@felixfontein
Copy link
Collaborator

@felixfontein "FATAL: Environment --docker fedora40 is unknown. " Looks like there are temporary runner issues. Nothing i can fix here.

Will get fixed by #9552.

@felixfontein
Copy link
Collaborator

Restarting CI.

@felixfontein felixfontein reopened this Jan 8, 2025
@felixfontein
Copy link
Collaborator

Hmm, looks like it didn't help. I guess you have to add another commit, or rebase with latest main.

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 8, 2025
@xrow
Copy link
Author

xrow commented Jan 9, 2025

@felixfontein Ok thanks for the help. Your turn again.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

hi @xrow thanks for your contribution.

Your changelog frag needs a small adjustment, other than that it LGTM.

@felixfontein
Copy link
Collaborator

Ping @Lunik @Shaps @lgatellier @marwatk @metanovii @nejch @scodeman @sh0shin @suukit @waheedi @zanssa - I hope that some of you might be familiar with keep_base_url and whether it is safe to enable it by default.

@nejch
Copy link
Contributor

nejch commented Jan 13, 2025

Sorry @felixfontein on vacation now so slipped my emails :)

I would personally make this configurable but disabled by default because it's almost like abusing the default external URL on the gitlab instance (gitlab doesn't support multiple external URLs yet). For the most part it works but some list endpoints and redirects might be broken this way IMO but it's been a while so not sure if that's improved.

Here are my thoughts on this from the initial PR for the library: python-gitlab/python-gitlab#2149 (comment)

@xrow
Copy link
Author

xrow commented Jan 13, 2025

@nejch

It looks like there is no impact on the external url, because of if not next_url.startswith(self._gl.url):

https://github.com/python-gitlab/python-gitlab/pull/2149/files#diff-cfa2b05f7b559e334405867e2c02564c88032f0dee928f61cba4b84d3d680886R1143

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request gitlab module_utils module_utils new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants