-
Notifications
You must be signed in to change notification settings - Fork 170
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
PLZ: add tolerations #428
base: main
Are you sure you want to change the base?
PLZ: add tolerations #428
Conversation
|
@aaron-lunsford-even Indeed, contribution guideline is still my WIP task; but more than that, it's the first time we get a PR for PLZ which is amazing! But we need to figure out the logistics. PLZ is very closely tied to Grafana Cloud, so new features almost always would require collaboration with Grafana Cloud team. For instance, here, I'll raise this issue and PR in internal planning, to coordinate how we can proceed here. From purely technical perspective, this PR has lots of similarities to adding custom |
@yorugac - Understood! Thanks for the feedback and all of the context around the PLZ flow. There's a lot more going on than I realized! I'll leave the PR as-is and keep an eye out for future updates. If there's anything you'd like me to do from my end, please let me know and I'll be happy to help out. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aaron-lunsford-even! Sorry for the delay. We had an internal discussion and it should be possible to go ahead with your PR as is 🎉 In case of tolerations, there is no real need to store them in our cloud: we can store it as part of PLZ resource and that's it. So basically as you did.
I'll have just a couple of small change requests here. Firstly, please make a rebase and sign a CLA: I can't merge anything otherwise. And secondly, if you run make generate
, there is .deepcopy.go
file that should be auto-updated - please add it.
That's the main thing. For testing, this is a straight-forward case so expanding unit test in pkg/testrun/plz_test.go
would be ideal 🙂
2cb0c29
to
8566329
Compare
Hey @yorugac, thanks for the update. I'll get those changes in and sign the CLA once I get the approval from my company. Should be soon 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaron-lunsford-even, thanks for the updates and for including Helm chart!
I've found one more thing that should be changed - please see the comment. Apologies I missed this during the first review 😅
That should be the last one + waiting for CLA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🙌
Thanks for the updates! Just waiting for the CLA then 🙂
Hi @aaron-lunsford-even, has it been possible to get the approval for CLA? 🙂 |
Fixes #427
I didn't see a contribution guideline and I noticed that PLZ tests were being added to the
testrun
package in #426, so I wasn't sure about adding them to this PR.