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

Feat. Add database naming strategy #38

Merged
merged 8 commits into from
Jul 30, 2023

Conversation

sleipnir
Copy link
Contributor

Includes the possibility of not forcing the use of the namespace in the name composition.

@mruoss
Copy link
Owner

mruoss commented Jul 29, 2023

Thanks @sleipnir
Seems something is off with the workflows... I will check it...

@mruoss
Copy link
Owner

mruoss commented Jul 29, 2023

Hey @sleipnir

  • The issues with the pipeline should be fixed on main, please rebase.
  • I find the code a bit cumbersome. Why not go shorter like this and spare the conversion logic:
using_prefix_strategy? = resource["spec"]["usingPrefixNamingStrategy"] != "false"
db_name = Database.name(resource, prefix_namespace: using_prefix_strategy?)

Note I've changed the second argument of Database.name/2 to be a keyword list (opts). I don't like flags without the scope as function vars.

I can take it from here if you want.

@sleipnir
Copy link
Contributor Author

sleipnir commented Jul 30, 2023

Perfect for me. I prefer to use pattern matching than chained map calls but ok.
I agree with using Keyword, I had started this way but I decided to change it because it was something very simple and I didn't see other options that could justify a list.

@sleipnir
Copy link
Contributor Author

sleipnir commented Jul 30, 2023

Hi @mruoss I believe that now everything is ok. Can you review it again please?

@mruoss
Copy link
Owner

mruoss commented Jul 30, 2023

Thanks @sleipnir, I'll merge this.

I will add a changelog entry and I might change the field from boolean to string before releasing it (i.e. databaseNamingStrategy: none | prefix_namespace instead of usingPrefixNamingStrategy: true | false where prefix_namespace is the default).

BTW: I'm prefixing the namespace per default so there's no conflicts if we have db resources with the same name in different namespaces. But if the user overrides this manually, that's okay I guess.

@mruoss mruoss merged commit 30b1c92 into mruoss:main Jul 30, 2023
12 of 13 checks passed
@sleipnir sleipnir deleted the feat/database-naming-strategy branch July 30, 2023 22:17
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.

None yet

3 participants