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

PLZ: add tolerations #428

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aaron-lunsford-even
Copy link

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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@aaron-lunsford-even aaron-lunsford-even changed the title Feat/plz add tolerations PLZ: add tolerations Jul 15, 2024
@yorugac
Copy link
Collaborator

yorugac commented Jul 17, 2024

@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, tolerations will need to be passed as part of API, like is done in the PR you linked:
https://github.com/grafana/k6-operator/pull/426/files#diff-e01d6388a2fbfc9c44d3a92e7db08cc546c954e1c3e4cf80e4bc8f7b51700c57
But that API must be modified first on the Cloud side.

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 image as is done in PR #426, so I think after #426 is merged, we should be able to work on this PR in similar fashion and "streamline" adding the changes. But still, it'll take some time, more changes made, some back and forth and, perhaps most importantly, e2e testing can be done only internally.
I'll come back to you once I have updates from the internal planning. Meanwhile, let me know how the above looks like to you 🙂

@aaron-lunsford-even
Copy link
Author

aaron-lunsford-even commented Jul 17, 2024

@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!

Copy link
Collaborator

@yorugac yorugac left a 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 🙂

@aaron-lunsford-even
Copy link
Author

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 🙂

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 🤞

Copy link
Collaborator

@yorugac yorugac left a 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.

api/v1alpha1/privateloadzone_types.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@yorugac yorugac left a 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 🙂

@yorugac
Copy link
Collaborator

yorugac commented Sep 10, 2024

Hi @aaron-lunsford-even, has it been possible to get the approval for CLA? 🙂

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.

Add support for tolerations to PrivateLoadZone objects
3 participants