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

Manually set global Git configuration in proc to sync templates with an existing repo #3243

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

asteflova
Copy link
Member

What changes are you introducing?

Attempting to update the Git global config for the foreman user fails due to insufficient permissions. Adding steps to manually create the config file as well as set the appropriate permissions on the file ensures that it gets updated as expected.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

https://issues.redhat.com/browse/SAT-27104

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

The proposed solution might be considered hacky by some but it looks like SAT-27349 should make the whole section obsolete in a future version.

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied
@asteflova asteflova changed the title Sat 27104 template sync git Manually set global Git configuration in proc to sync templates with an existing repo Aug 28, 2024
@asteflova
Copy link
Member Author

@adamruzicka Can you please review?

@asteflova asteflova added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective and removed Not yet reviewed labels Aug 28, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I wonder if it makes sense to ship an empty config file with the correct permissions by default so I opened theforeman/foreman-packaging#11180. There's some room for discussion of that implementation and we don't have it available everywhere immediately so it doesn't block merging this.

@adamruzicka
Copy link
Contributor

A little bit of context: we're planning some changes to foreman_templates so that the http proxy configured in foreman's settings would be implicitly used, without the user having to go faff around with git config so the whole procedure along with the changes proposed here may become irrelevant.

I wonder if it makes sense to ship an empty config file with the correct permissions by default so I opened

We considered that option but decided against it, considering this is only temporary solution until the changes in f-templates land. But since you already went the extra mile, thank you

@ekohl
Copy link
Member

ekohl commented Aug 28, 2024

A little bit of context: we're planning some changes to foreman_templates so that the http proxy configured in foreman's settings would be implicitly used, without the user having to go faff around with git config so the whole procedure along with the changes proposed here may become irrelevant.

The HTTP proxy configuration still doesn't set the trusted CA certificates. If you have a self signed certificate for your private gitlab then that doesn't help you, right? Or should we recommend setting up the trusted certificate system wide and avoid the specific git configuration?

@asteflova
Copy link
Member Author

asteflova commented Aug 29, 2024

Agreed, fixing the code would be preferable to making users do the legwork of manual configuration, but considering the context (explained in the PR description and by Adam above), my recommendation was to fix this in the docs.

@ekohl Since you opened the foreman-packaging PR (thanks for that, by the way!), can you please help me figure out which branches should the doc fix go to? Should I cherry-pick only to 3.12 and below because for 3.13 and above, the file will be shipped by default?

@adamruzicka
Copy link
Contributor

The HTTP proxy configuration still doesn't set the trusted CA certificates. If you have a self signed certificate for your private gitlab then that doesn't help you, right? Or should we recommend setting up the trusted certificate system wide and avoid the specific git configuration?

I wanted to keep this brief, but since you're asking about it, I'll elaborate. The full plan is to offer an option to pick a proxy from a list of available proxies in the template sync form. The list will be populated with the default proxy as set in settings and all the currently visible proxies configured in infrastructure > http proxies, but will default to the one in settings. For the ones in infrastructure > http proxies, you can paste in a CA cert in case you need it.

@ekohl
Copy link
Member

ekohl commented Aug 29, 2024

@ekohl Since you opened the foreman-packaging PR (thanks for that, by the way!), can you please help me figure out which branches should the doc fix go to? Should I cherry-pick only to 3.12 and below because for 3.13 and above, the file will be shipped by default?

I'd suggest to merge it first to master, pick it down to all branches you want. Once the packaging change becomes available we'll submit a docs update and then pick it to the same branches where the packaging update was picked to. We should also keep downstream in mind for that, so I probably wouldn't pick it back further than 3.10.

@asteflova
Copy link
Member Author

I'm going to assume there are no technical issues with the current state of this PR so I'm setting tech review done. Style review would be appreciated now.

@asteflova asteflova added tech review done No issues from the technical perspective and removed Needs tech review Requires a review from the technical perspective labels Sep 2, 2024
@asteflova
Copy link
Member Author

[...] Once the packaging change becomes available we'll submit a docs update [...]

Another assumption I'm going to make: "We'll submit a docs update" means that you intend to track this on your side. If that's not the case, let me know and I'll file a Jira for the docs team and/or myself.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

one tiny suggestion, rest LGTM. Feel free to merge with or without.

Co-authored-by: Maximilian Kolb <[email protected]>
@asteflova asteflova added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective labels Sep 3, 2024
@asteflova asteflova merged commit 9db6c8b into theforeman:master Sep 3, 2024
9 checks passed
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
asteflova added a commit that referenced this pull request Sep 3, 2024
* Manually add Git config for template sync

Without these steps, `sudo -u foreman git config --global` fails with
this error:

error: could not lock config file /usr/share/foreman/.gitconfig: Permission denied

* Minor edits and clarifications

---------

Co-authored-by: Maximilian Kolb <[email protected]>
(cherry picked from commit 9db6c8b)
@asteflova
Copy link
Member Author

Merged to "master" and cherry-picked:

89c1a95..a556446 3.12 -> 3.12
64fe5db..f3230b9 3.11 -> 3.11
3287c30..5e6337a 3.10 -> 3.10
57dc4c0..56f0d67 3.9 -> 3.9
68d15e2..58c89b3 3.8 -> 3.8
2d58662..db84943 3.7 -> 3.7
763d727..cfd432d 3.6 -> 3.6
649bfa2..ad13123 3.5 -> 3.5

@ekohl
Copy link
Member

ekohl commented Sep 3, 2024

Another assumption I'm going to make: "We'll submit a docs update" means that you intend to track this on your side. If that's not the case, let me know and I'll file a Jira for the docs team and/or myself.

I added a reminder on the packaging PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants