Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

remove hardcoded rootPath values#1874

Merged
matt0x6F merged 4 commits intogomods:mainfromJeansen:fix-config-overwrites
Apr 13, 2024

Conversation

Jeansen
Copy link
Contributor

What is the problem I am trying to address?

Systemd script had some hard-coded values overwriting the config.toml settings. I've removed them and added some dynamic replacements in the athens.service

How is the fix applied?

?

What GitHub issue(s) does this PR fix or close?

Fixes#1798

@JeansenJeansen requested a review froma team as acode ownerMay 29, 2023 09:00
@JeansenJeansenforce-pushed thefix-config-overwrites branch from6b9ddb8 toafc374fCompareMay 29, 2023 09:00
@Jeansen
Copy link
ContributorAuthor

Jeansen commentedMay 29, 2023
edited
Loading

I believe the install shell script can be much more robust in some places and some variables could be extracted to the top. Currently, the scrip also expects some files in the right place, instead of being relative to its location. I'll do that cleanup work in another PR. This PR's sole purpose is to remove the hard-coded values. One step at a time ;-)

@DrPsychickDrPsychick added this to the0.13.x thereafter milestoneJun 23, 2023
Comment on lines -48 to -49
sudo chown www-data $ATHENS_DISK_STORAGE_ROOT
sudo chgrp www-data $ATHENS_DISK_STORAGE_ROOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't thechown andchgrp commands be kept in order to still make surewww-data owns theReadWritePaths?

@Ullaakut
Copy link
Collaborator

Hi@Jeansen ! Are you still interested in getting this merged?

@Jeansen
Copy link
ContributorAuthor

Hi@Ullaakut. Yes, I am. But I am not sure, how to proceed...

@matt0x6F
Copy link
Contributor

Hi@Ullaakut. Yes, I am. But I am not sure, how to proceed...

Heya! Do you mind ELI5'ing these changes as well as your long term plan? I'm actually pretty unsure of how all of this should work. It might also be helpful to have a main issue for the long term plan as well as some sub-issues since you want to tackle this in phases.

Ullaakut reacted with thumbs up emoji

@Jeansen
Copy link
ContributorAuthor

Heya! Do you mind ELI5'ing these changes as well as your long term plan? I'm actually pretty unsure of how all of this should work. It might also be helpful to have a main issue for the long term plan as well as some sub-issues since you want to tackle this in phases.

I'll try! This PR removes hard-coded values. The only place of truth should the the config TOML file. If this is in agreement with you (all), then that's what this PR is about.

To make things more robust, I see some more possible improvements in the install script. But since this is my first PR in the project, I first wanted to get some idea of how this project "ticks". Anyway, if you think my ideas are worth a shot, I could create another PR so you see, where I'd like to go.

But as it stands, it is all about having as much hard-coded values removed or extracted to only one place and making the installation follow Postel's Law.

@matt0x6F
Copy link
Contributor

That makes sense! Thanks for the breakdown.

Does it make sense to ensure whatever directory the user has supplied that it fits the permissions we need and that it actually exists? I'm referring tothis being removed:

@Jeansen
Copy link
ContributorAuthor

I think users should be aware of the fact that they are missing the right permissions and be forced to set them explicitly. Convention over configuration is a great concept. But with respect to permissions I'd be careful ;-)

matt0x6F reacted with thumbs up emoji

@matt0x6F
Copy link
Contributor

Thanks for that bit of wisdom.

Last thing, do you think anythinghere needs changing?

@Jeansen
Copy link
ContributorAuthor

@matt0x6F I'll have a look at it this WE and give you my feedback then.

matt0x6F reacted with thumbs up emoji

@Jeansen
Copy link
ContributorAuthor

I checked the helm repo. It looks fine to me, so far. But I did not dive into details too far, to be honest.

Anyway, there you have it, too. The user ist set to 1000 if not run as root. And theemptyDir volume ist mountet with the same user/group since there is not explicitfsGroup set. So, no access problems here.

Also, helm templates kind of serve as a on-the-fly config file by creating all the relevant environment variables which - if applicable - overwrite settings from the config file. Regarding this I'd strongly advice against being able to set a user/group through environment variables. But I would put theathens user and group as a default value inconfig.tom. And like I wrote already, if a non-cluster setup fails, then a user can either change user and group settings or create theathens user and group.

I also saw the issue requesting an option to add a customconfig.toml file. I'd also recommend against it. If anyone needs a custom config file in place, one can always create a custom Docker image base on the athens image. Otherwise I think everything I see so far is aligned with a 12-factor app design which also means that all relevant settings can be set through environment variables.

matt0x6F reacted with thumbs up emoji

@matt0x6F
Copy link
Contributor

That makes sense. I've got a couple PRs to merge later today so this one's on my list!

Jeansen reacted with laugh emoji

@Jeansen
Copy link
ContributorAuthor

I believe the install shell script can be much more robust in some places and some variables could be extracted to the top. Currently, the scrip also expects some files in the right place, instead of being relative to its location. I'll do that cleanup work in another PR. This PR's sole purpose is to remove the hard-coded values. One step at a time ;-)

I'll go for the next phase then, do some more improvements, cleanup etc - if that's OK?

@matt0x6Fmatt0x6F merged commitf969e03 intogomods:mainApr 13, 2024
11 checks passed
DrPsychick referenced this pull request in gomods/athens-chartsApr 16, 2024
[![MendRenovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)This PR contains the following updates:| Package | Update | Change ||---|---|---|| [gomods/athens](https://togithub.com/gomods/athens) | patch |`v0.13.1` -> `v0.13.3` |---### Release Notes<details><summary>gomods/athens (gomods/athens)</summary>### [`v0.13.3`](https://togithub.com/gomods/athens/releases/tag/v0.13.3)[CompareSource](https://togithub.com/gomods/athens/compare/v0.13.2...v0.13.3)#### What's Changed- Update README.md by[@&#8203;computerscienceiscool](https://togithub.com/computerscienceiscool)in[https://github.com/gomods/athens/pull/1932](https://togithub.com/gomods/athens/pull/1932)- update-go-pkg(deps): bump github.com/stretchr/testify from 1.8.4 to1.9.0 by [@&#8203;dependabot](https://togithub.com/dependabot) in[https://github.com/gomods/athens/pull/1933](https://togithub.com/gomods/athens/pull/1933)- Upgrade logrus from 1.7.0 => 1.9.3 by[@&#8203;matt0x6F](https://togithub.com/matt0x6F) in[https://github.com/gomods/athens/pull/1934](https://togithub.com/gomods/athens/pull/1934)- should use errors.AsErr to extract and detect errors.Error by[@&#8203;kkHAIKE](https://togithub.com/kkHAIKE) in[https://github.com/gomods/athens/pull/1936](https://togithub.com/gomods/athens/pull/1936)- correcting the misuse of the context in the copyContextWithCustomTime…by [@&#8203;kkHAIKE](https://togithub.com/kkHAIKE) in[https://github.com/gomods/athens/pull/1941](https://togithub.com/gomods/athens/pull/1941)- remove hardcoded rootPath values by[@&#8203;Jeansen](https://togithub.com/Jeansen) in[https://github.com/gomods/athens/pull/1874](https://togithub.com/gomods/athens/pull/1874)#### New Contributors-[@&#8203;computerscienceiscool](https://togithub.com/computerscienceiscool)made their first contribution in[https://github.com/gomods/athens/pull/1932](https://togithub.com/gomods/athens/pull/1932)- [@&#8203;kkHAIKE](https://togithub.com/kkHAIKE) made their firstcontribution in[https://github.com/gomods/athens/pull/1936](https://togithub.com/gomods/athens/pull/1936)- [@&#8203;Jeansen](https://togithub.com/Jeansen) made their firstcontribution in[https://github.com/gomods/athens/pull/1874](https://togithub.com/gomods/athens/pull/1874)**Full Changelog**:gomods/athens@v0.13.2...v0.13.3### [`v0.13.2`](https://togithub.com/gomods/athens/releases/tag/v0.13.2)[CompareSource](https://togithub.com/gomods/athens/compare/v0.13.1...v0.13.2)#### What's Changed- Send standard logger's output to logrus by[@&#8203;mikesep](https://togithub.com/mikesep) in[https://github.com/gomods/athens/pull/1912](https://togithub.com/gomods/athens/pull/1912)- chore: fix broken links to 'absolutely everybody' blog post by[@&#8203;darrylblake](https://togithub.com/darrylblake) in[https://github.com/gomods/athens/pull/1914](https://togithub.com/gomods/athens/pull/1914)- update-github-action(deps): bump golangci/golangci-lint-action from 3to 4 by [@&#8203;dependabot](https://togithub.com/dependabot) in[https://github.com/gomods/athens/pull/1915](https://togithub.com/gomods/athens/pull/1915)- update-go-pkg(deps): bump github.com/gorilla/mux from 1.6.2 to 1.8.1by [@&#8203;dependabot](https://togithub.com/dependabot) in[https://github.com/gomods/athens/pull/1917](https://togithub.com/gomods/athens/pull/1917)- update-go-pkg(deps): bump github.com/stretchr/testify from 1.8.1 to1.8.4 by [@&#8203;dependabot](https://togithub.com/dependabot) in[https://github.com/gomods/athens/pull/1918](https://togithub.com/gomods/athens/pull/1918)- update-go-pkg(deps): bump go.etcd.io/etcd/api/v3 from 3.5.9 to 3.5.12by [@&#8203;dependabot](https://togithub.com/dependabot) in[https://github.com/gomods/athens/pull/1919](https://togithub.com/gomods/athens/pull/1919)- Fix Markdown link in Storage docs by[@&#8203;chriskuehl](https://togithub.com/chriskuehl) in[https://github.com/gomods/athens/pull/1922](https://togithub.com/gomods/athens/pull/1922)- Use quotes for args by[@&#8203;matt0x6F](https://togithub.com/matt0x6F) in[https://github.com/gomods/athens/pull/1925](https://togithub.com/gomods/athens/pull/1925)- Add log formatting settings by[@&#8203;matt0x6F](https://togithub.com/matt0x6F) in[https://github.com/gomods/athens/pull/1926](https://togithub.com/gomods/athens/pull/1926)- upgrade mongodb driver by[@&#8203;xytan0056](https://togithub.com/xytan0056) in[https://github.com/gomods/athens/pull/1928](https://togithub.com/gomods/athens/pull/1928)- update-go-pkg(deps): bump github.com/lib/pq from 1.10.7 to 1.10.9 by[@&#8203;dependabot](https://togithub.com/dependabot) in[https://github.com/gomods/athens/pull/1923](https://togithub.com/gomods/athens/pull/1923)- Rework logging defaults by[@&#8203;matt0x6F](https://togithub.com/matt0x6F) in[https://github.com/gomods/athens/pull/1927](https://togithub.com/gomods/athens/pull/1927)#### New Contributors- [@&#8203;darrylblake](https://togithub.com/darrylblake) made theirfirst contribution in[https://github.com/gomods/athens/pull/1914](https://togithub.com/gomods/athens/pull/1914)- [@&#8203;chriskuehl](https://togithub.com/chriskuehl) made their firstcontribution in[https://github.com/gomods/athens/pull/1922](https://togithub.com/gomods/athens/pull/1922)- [@&#8203;matt0x6F](https://togithub.com/matt0x6F) made their firstcontribution in[https://github.com/gomods/athens/pull/1925](https://togithub.com/gomods/athens/pull/1925)**Full Changelog**:gomods/athens@v0.13.1...v0.13.2</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once youare satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/gomods/athens-charts).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjMwMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@UllaakutUllaakutUllaakut left review comments

@matt0x6Fmatt0x6Fmatt0x6F approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.3
Development

Successfully merging this pull request may close these issues.

Systemd setup has hard-coded ATHENS_DISK_STORAGE_ROOT
5 participants
@Jeansen@Ullaakut@matt0x6F@DrPsychick@manugupt1

[8]ページ先頭

©2009-2025 Movatter.jp