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.

Sign up for GitHub

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

Jump to bottom

Make it so that conf/*.sh can be trivially overridden by the user #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make it so that conf/*.sh can be trivially overridden by the user #91

wants to merge 1 commit into from

Conversation

ctm
Copy link

@ctm ctm commented Aug 6, 2019 •
edited

Closes #90

Prior to this commit, editing the conf/.conf files resulted in
changed configuration parameters, but editing the conf/.sh files did
not. Furthermore, there was a comment in the conf/*.sh files that
could be misconstrued.

With this commit the conf/.sh and conf/.conf files behave the same.
conf/*.sh files can be edited and then docker -v can be used to bind
mount the conf directory and the changes will take effect.

Checklist

  • Implementation
  • Tests
  • Docs

@ctm
Make it so that conf/*.sh can be trivially overridden by the user
2c9cb63 
Closes #90

Prior to this commit, editing the conf/*.conf files resulted in
changed configuration parameters, but editing the conf/*.sh files did
not.  Furthermore, there was a comment in the conf/*.sh files that
could be misconstrued.

With this commit the conf/*.sh and conf/*.conf files behave the same.
conf/*.sh files can be edited and then docker -v can be used to bind
mount the conf directory and the changes will take effect.
@SISheogorath
Copy link
Contributor

This all can be solved by fixing the bug introduced by the rewrite to multi-staged builds that moved the workdir from /inspircd to /.

Also the warnings were added to the files to prevent users from modifying those files within the repository because they thought those would be mounted to the image. Within the image or before you build the image, you can do with those files what you want :)

@HumorBaby
Copy link
Contributor

HumorBaby commented Aug 6, 2019 •
edited

This all can be solved by fixing the bug introduced by the rewrite to multi-staged builds that moved the workdir from /inspircd to /.

Essentially, yes! 😁 I noticed this the other day when trying to use 'conf.d', but it was not doing anything. I am laying out a PR for this (and other slightly related issues) at https://github.com/HumorBaby/inspircd-docker/commits/fix-refactor-tests; specifically, I think
a4c1e52 will be the easiest fix to this issue (IMO)

@HumorBaby
Copy link
Contributor

Also, sorry @ctm!

I am laying out a PR for this (and other slightly related issues) at https://github.com/HumorBaby/inspircd-docker/commits/fix-refactor-tests;

is why I jumped on replying to #90 so fast 🐱 I had been tracing through the .sh/.conf steps for a few days and thought I had a solution, and this PR may (or may not... idk yet) undo some of that. But that shouldn't stop this PR if that's what's desired, of course!

@ctm
Copy link
Author

ctm commented Aug 6, 2019

I have no desire to see this PR merged, per-se. I provided it more as a concrete example of what I was (perhaps poorly) describing. I was surprised that the .conf files within $(INSPIRCD_ROOT) were referring to .sh files above $(INSPIRCD_ROOT) and wasn't sure if that was a deliberate decision or if it came about by accident.

I misconstrued the comments in the .sh files and initially thought that they were implying that the .sh files were executed during the build process. That lead to some head-scratching and hair-pulling because it looked to me like they were executed at runtime (and they are!).

Eventually I realized there were two copies of the .sh files, the ones that were used but weren't overlayed with -v and the ones that were ignored but were within $(INSPIRCD_ROOT). This PR is one way (but probably not the best way) to have just one copy of the .sh files and have them show up inside $(INSPIRCD_ROOT).

I have no doubt that SISheogorath has better ideas than this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

conf/*.sh files are (unnecessarily?) tricky to override
3 participants
@ctm @SISheogorath @HumorBaby