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

Proposal for a Helm Deployment goal implementation #15882

Merged
merged 140 commits into from Aug 10, 2022

Conversation

alonsodomin
Copy link
Contributor

This is the PoC implementation for the proposal of a Helm Deployment goal. Design document can be found in:

https://docs.google.com/document/d/17wZRjiIxM5918ybC_0EnKY42qwd47-1ZporBdua8vRQ/edit?usp=sharing

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks! I didn't review the helm implementation, only the deploy goal itself: let us know when you're ready to move forward with that portion.

src/python/pants/core/goals/deploy.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/deploy.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/deploy.py Outdated Show resolved Hide resolved
@alonsodomin
Copy link
Contributor Author

alonsodomin commented Jul 21, 2022

Reworked the implementation of the core deploy goal.

The new implementation obtains a single DeployProcess per DeployFieldSet and goes one by one finding their publish dependencies, which will be published before invoking the deploy process.

If publishing of any of the deploy dependencies fails, then that deployment is skipped. In previous implementation, the whole goal would exit at that point.

@alonsodomin alonsodomin requested a review from tdyas July 27, 2022 11:00
@tdyas
Copy link
Contributor

tdyas commented Jul 28, 2022

I prefer to see the core deploy goal land as a separate PR, mainly to make it easier to review. This PR is a bit large as it is. Thoughts?

@alonsodomin
Copy link
Contributor Author

@tdyas yeah, perfectly normal to understand, the implemented machinery in the Helm side has quite a few moving pieces.

Will create a PR for the core deploy goal only

@alonsodomin
Copy link
Contributor Author

core deploy implementation has been submitted as the PR #16335 separately

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I have only reviewed documentation for now. I'll check out implementation details another day.

I have several wording suggestions for the docs and a few questions about how things work.

docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
@alonsodomin
Copy link
Contributor Author

first of all, thanks a lot for proof reading the documentation, this was very helpful!

@alonsodomin alonsodomin force-pushed the helm_install branch 3 times, most recently from e8f14f3 to 0a9eae6 Compare August 2, 2022 14:51
stuhood pushed a commit that referenced this pull request Aug 3, 2022
Extracted from #15882, this provides with the minimal implementation of a `deploy` goal.
@alonsodomin
Copy link
Contributor Author

the core deploy implementation has been removed from this PR as it has already been merged in a separate PR.

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks! Solid work.

docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
docs/markdown/Helm/helm-deployments.md Outdated Show resolved Hide resolved
pants.toml Outdated Show resolved Hide resolved
src/python/pants/backend/helm/subsystems/helm.py Outdated Show resolved Hide resolved
src/python/pants/backend/helm/subsystems/k8s_parser.py Outdated Show resolved Hide resolved
src/python/pants/backend/helm/target_types.py Outdated Show resolved Hide resolved
src/python/pants/backend/helm/utils/docker.py Outdated Show resolved Hide resolved
@cognifloyd
Copy link
Member

Does this support deploying an external chart? Or does the chart have to be embedded in the repo?

@alonsodomin alonsodomin merged commit bac3e04 into pantsbuild:main Aug 10, 2022
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
Extracted from pantsbuild#15882, this provides with the minimal implementation of a `deploy` goal.
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
@stuhood
Copy link
Sponsor Member

stuhood commented Oct 21, 2022

Does this support deploying an external chart? Or does the chart have to be embedded in the repo?

I believe that #16478 added support for external charts?

@alonsodomin
Copy link
Contributor Author

Yes, deploying third party charts was in that PR.

@huonw huonw mentioned this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants