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

Adding optional CONFIG_PATH env var support #588

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

dolan-a
Copy link
Contributor

@dolan-a dolan-a commented Feb 14, 2024

This PR adds support for defining the CONFIG_PATH settings through an environment variable. I'm not in love with the env var's name since it is pretty generic, but I stuck with the naming convention of the other variables in env.js

@decyjphr
Copy link
Collaborator

Strange - this PR is basically undoing a previous change 1850938 which removed this flag.

@dolan-a
Copy link
Contributor Author

dolan-a commented Feb 26, 2024

Thanks @decyjphr for pointing out the other PR. I looked at the comments on #351, and I think my PR is still worth considering. A few specific points to address:

  • Probot's suggestion of using .github for config files: As @martinm82 noted, Probot's best-practices docs say to store configs in the .github directory of repos. I think this suggestion is more appropriate for GitHub Apps that would load repo-specific configurations from the repo directly. Since safe-settings instead stores all configurations in an "admin" repo, I don't think this best-practice is as relevant.

  • Updating all hardcoded ".github" directory references: The author of feat: allow different admin repo by env variable #351 rightly noted that there are several spots in the code that are hardcoding the config-path directory. I've since updated this PR to use env.CONFIG_PATH instead. I left most of the README as is -- other than adding a note about the CONFIG_PATH env being an optional param.

Ideally we'd have some unit tests to ensure that the repo and sub-org overrides are being loaded correctly when overriding CONFIG_PATH. It wasn't clear to me how best to go about this though, so if anyone has suggestions, let me know.

Copy link
Collaborator

@decyjphr decyjphr 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 the changes.

@decyjphr decyjphr merged commit d4ea951 into github:main-enterprise Feb 28, 2024
1 check passed
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.

2 participants