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 unique prefix to resource names #90

Merged
merged 23 commits into from
Apr 30, 2024
Merged

feat: add unique prefix to resource names #90

merged 23 commits into from
Apr 30, 2024

Conversation

davidcavazos
Copy link
Collaborator

@davidcavazos davidcavazos commented Mar 27, 2024

Fixes: #84

Internal: cl/629129167

Prefixing all resources with knowledge-base-.

This should avoid name clashes with other solutions.

@davidcavazos davidcavazos requested review from a team as code owners March 27, 2024 13:50
Copy link
Contributor

@NimJay NimJay 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 doing this, David! :)

Looks like knowledge-base-trigger-sa-9c43dc is too long (based on Terraform failures in GitHub checks).

What if we used kb- as a prefix or something else that's shorter?
I believe we need to stay at or below 30 characters.

@NimJay
Copy link
Contributor

NimJay commented Mar 27, 2024

Also, feel free to ignore the terraform-genai-knowledge-base-int-trigger GitHub check failures in this pull-request.
Getting webhook / test (3.12) (pull_request) to pass should be adequate for this pull-request.

@davidcavazos
Copy link
Collaborator Author

Okay, just knowledge-base-trigger-sa is 25 characters long. So what I'll do is that if unique_names is set to false which is what users will always do, we'll use the knowledge-base- prefix since it's nicer looking and more explicit. The unique_names are used for tests so that we can run multiple concurrent tests from different PRs, so those can use a kb- prefix to keep them shorter.

@NimJay
Copy link
Contributor

NimJay commented Mar 27, 2024

"The unique_names are used for tests so that we can run multiple concurrent tests from different PRs"

Ah, I see what you mean: the webhook / test (3.12) check uses the same project for each pull-request.
For what it's worth, the terraform-genai-knowledge-base-int-trigger GitHub check does a terraform apply and terraform destroy test and creates a new project per pull-request (so we don't have to worry about parallel PRs).
Should we eventually remove webhook / test (3.12) if we get terraform-genai-knowledge-base-int-trigger GitHub check working in #88?
I'd love to hear your thoughts. (But of course, this discussion is out-of-scope for this PR.)

@davidcavazos
Copy link
Collaborator Author

davidcavazos commented Mar 27, 2024

I still like having the webhook tests since the other ones just test the deployment and the webhook tests check the logic. Plus they run a lot faster so it's easier to iterate over them.

They also do more granular linting and type checking.

Copy link
Contributor

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

I'm approving this for correctness and so you can merge as soon as you've decided to merge.
But I'll leave it up to you to coordinate with Karl/Don on whether we should merge this this week (before Console code freeze) or next week (during Console code freeze).

(Updates to the Neos tutorial and Terraform inside GitHub aren't impacted by code freeze. So we can delay this change until next week.)

@github-actions github-actions bot added feature and removed feature labels Mar 27, 2024
@davidcavazos
Copy link
Collaborator Author

davidcavazos commented Mar 27, 2024

We're going to pause this for the time being, but I'll leave this open to have it ready to go. We can probably merge it next week.

@github-actions github-actions bot added feature and removed feature labels Apr 25, 2024
@davidcavazos davidcavazos added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 29, 2024
@github-actions github-actions bot added feature and removed feature labels Apr 29, 2024
@davidcavazos davidcavazos merged commit 50ce35e into main Apr 30, 2024
9 checks passed
@davidcavazos davidcavazos deleted the unique-names branch April 30, 2024 18:13
@davidcavazos davidcavazos removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[terraform] Rename resources to avoid clashes with other solutions
2 participants