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

Support namespace specification #115

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

LittleWat
Copy link
Contributor

This PR attempts to close #114

This would be the standard way to support namespace specification.

@cla-bot cla-bot bot added the cla-signed label Dec 20, 2023
@LittleWat
Copy link
Contributor Author

@hashhar sorry to ask you but could you kindly review this, please...? 🙏

@hashhar hashhar requested a review from mosabua January 9, 2024 06:16
@LittleWat LittleWat force-pushed the support-namespace-specification branch from 08e7cc1 to ce8d856 Compare March 12, 2024 05:26
@mosabua
Copy link
Member

mosabua commented Mar 12, 2024

This looks good to me. It would be good if we can document this as part of the frigate generated doc or maybe in a separate markdown file in a docs folder or so. Over time I want to get more docs in that folder and into a site

@mosabua
Copy link
Member

mosabua commented Mar 12, 2024

Also maybe @radek-starburst @nineinchnick or @losipiuk can chime in.

And @LittleWat I very much assume you tested and verified this already.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good but we need to do some minimal docs.

@LittleWat LittleWat force-pushed the support-namespace-specification branch from ce8d856 to c0093fd Compare March 13, 2024 02:41
@LittleWat
Copy link
Contributor Author

LittleWat commented Mar 13, 2024

@mosabua thank you for your review!

And @LittleWat I very much assume you tested and verified this already.

Yes, I've tested this on my Linux ec2 machine. It worked as expected.

Looks good but we need to do some minimal docs.

Updated the README.md on how to use the helm template command by specifying the namespace!

Could you review this again, please...? 🙏

@mosabua
Copy link
Member

mosabua commented Mar 13, 2024

Please rebase

This PR attempts to close trinodb#114

This would be the standard way to support namespace specification.
@LittleWat LittleWat force-pushed the support-namespace-specification branch from c0093fd to d0ab859 Compare March 14, 2024 02:08
@LittleWat
Copy link
Contributor Author

@mosabua rebased!

@@ -3,6 +3,7 @@ apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
name: {{ template "trino.worker" . }}
namespace: {{ .Release.Namespace }}
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you do not specify namespace when installing helm? Will that resolve to default?

Copy link
Member

Choose a reason for hiding this comment

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

I asked some clarifying questions in the issue: #114 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your reviews!

what happens if you do not specify namespace when installing helm? Will that resolve to default?

Yes, it will resolve to default namespace.

@mosabua
Copy link
Member

mosabua commented Mar 14, 2024

I think this is fine as long as we can get confirmation that this works transparently as before if no namespace parameter is passed in. I am not sure it does ...

@LittleWat
Copy link
Contributor Author

I think this is fine as long as we can get confirmation that this works transparently as before if no namespace parameter is passed in. I am not sure it does ...

Thank you for your comment! if no namespace parameter is passed in, the namespace is set to default regardless of whether to include this difference or not. so it would be okay.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Good with me.

@losipiuk losipiuk merged commit 5720e83 into trinodb:main Mar 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Support namespace specification
4 participants