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

Improve inventory cleanup on the first time setup workflow #40

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

Ndpnt
Copy link
Member

@Ndpnt Ndpnt commented Mar 8, 2024

Add cleaning of encrypted values

@Ndpnt Ndpnt requested a review from clementbiron March 8, 2024 10:58
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Good catch!
But, just like we forgot to update this workflow file when we updated the inventory, we will again forget to update it on the next change 😕
Could we maybe keep that default file alongside the actual inventory? Or at least as a file alongside the workflow in the .github folder?

@Ndpnt
Copy link
Member Author

Ndpnt commented Mar 8, 2024

Could we maybe keep that default file alongside the actual inventory? Or at least as a file alongside the workflow in the .github folder?

In my opinion, this solution does not provide any additional visibility to the default inventory.

Could we maybe keep that default file alongside the actual inventory?

This one seems to be a better idea.
But after trying locally, my opinion is that this solution will pollute the demo repository, which will start to look more like a template than a demo repository, and that's what we wanted to avoid when we decided to merge demo and template repositories. I also think this raises the question of doing the same for the README and for config/production.json.

Finally, I'm not really convinced that this will prevent us from forgetting to update it.

I prefer to live with the current situation, and I think it's our responsibility to bear in mind that this repository is not a collection like any other, but that it also serves as a template.

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Okay, you convinced me. I am sure we will forget again, but I agree this is a risk more worth taking than polluting the demo 👍

@Ndpnt Ndpnt merged commit 84bc6cc into main Mar 8, 2024
4 checks passed
@Ndpnt Ndpnt deleted the update-setup-workflow branch March 8, 2024 15:35
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