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

refactor: standardize variable file naming and merge glob patterns #732

Open
wants to merge 4 commits into
base: main-enterprise
Choose a base branch
from

Conversation

PendaGTP
Copy link
Contributor

@PendaGTP PendaGTP commented Jan 7, 2025

Changes

No functional changes

  • Extracted sub-org config pattern into Settings.SUB_ORG_PATTERN
  • Extracted repo config pattern into Settings.REPO_PATTERN
  • Rename Settings.FILE_NAME to Settings.FILE_PATH for better clarity and consistency
  • Rename env.DEPLOYMENT_CONFIG_FILE to env.DEPLOYMENT_CONFIG_FILE_PATH
    • I didn't rename env var since this will cause a breaking change

Note: We could also consider to add env var DEPLOYMENT_CONFIG_FILE_PATH to support both old and new naming in the future. This would allow us to introduce it in a minor version while maintaining backward compatibility. Removing the old env vars would then be a breaking change for a future major version.

Why?

I'm currently deep dive into the code base to try fix some issues and doing other refactor to help us contribute more easily.
I will try to submit PR with some small changes instead of big one and no breaking change if possible.
Let me know if this approach works for you - I want to be helpful, not disruptive.

@PendaGTP PendaGTP changed the title Refactor configuration files naming/handling refactor: standardize variable file naming and merge glob patterns Jan 7, 2025
@HagegeR
Copy link

HagegeR commented Jan 8, 2025

I also felt this inconsistency when using safe-settings as a github action, I think if this gets merged it'll make the first time users life a lot easier

@PendaGTP
Copy link
Contributor Author

PendaGTP commented Jan 8, 2025

I also felt this inconsistency when using safe-settings as a github action, I think if this gets merged it'll make the first time users life a lot easier

To clarify, this PR currently only affects the backend implementation and does not change any environment variables that users need to configure.

In the futur, I think it would be beneficial to also rename some exposed environment variables to make them more intuitive.

However, I believe it might be too early to implement theses changes now. I would be better to include them with other breaking changes (new major version) in a future release to avoid frustating users with frequent breaking changes.
As an alternative, we could improve the documentation to make the current environment variable names and their purposes clearer to users. Personally, I find the current environment variable documentation clear enough, but if you have any suggestions for improvements, please feel free to share them.

This is just my current opinion, so open to other perpectives.

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