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.

Sign up for GitHub

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

Jump to bottom

Feat: helm template support + helm repo vcs option support #658

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Feat: helm template support + helm repo vcs option support #658

merged 3 commits into from
Jun 13, 2023

Conversation

TomerHeber
Copy link
Collaborator

Issue & Steps to Reproduce / Feature Request

resolves #655

Solution

  1. Updated API structs.
  2. Added required fields in the schema.
  3. Added unit tests.
  4. Added integration tests.

@TomerHeber TomerHeber self-assigned this Jun 9, 2023
@TomerHeber TomerHeber requested a review from yaronya June 9, 2023 13:22
tomporat247
tomporat247 reviewed Jun 11, 2023
View reviewed changes
Copy link

@tomporat247 tomporat247 left a comment

Choose a reason for hiding this comment

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

Looking good overall.
Asked some questions and wrote a few comments

env0/resource_template_test.go Show resolved Hide resolved
env0/resource_template.go Show resolved Hide resolved
env0/resource_template.go Show resolved Hide resolved
env0/resource_template.go Show resolved Hide resolved
@TomerHeber
Copy link
Collaborator Author

@tomporat247 - thanks for all the feedback. added a new commit 72008cc

Regarding your comment here: #658 (comment)
I did not make any changes to that. But if you think it's important, I can modify that.

tomporat247
tomporat247 reviewed Jun 12, 2023
View reviewed changes
client/template.go Outdated
@@ -140,6 +144,16 @@ func (payload TemplateCreatePayload) Validate() error {
return fmt.Errorf("file_name cannot be set when template type is: %s", payload.Type)
}

if payload.IsHelmRepository {
Copy link

Choose a reason for hiding this comment

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

This code still allows the user to set HelmChartName without IsHelmRepository and for non helm templates.

Not too serious but it still allows it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added additional conditionals.

tomporat247
tomporat247 approved these changes Jun 12, 2023
View reviewed changes
Copy link

@tomporat247 tomporat247 left a comment

Choose a reason for hiding this comment

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

Looking good overall, I left one more comment but I think it's not too important as that problem probably happens in other use cases here as well

@github-actions github-actions bot added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Jun 12, 2023
@tomporat247
Copy link

Note that your unit tests are failing

@TomerHeber
Copy link
Collaborator Author

Note that your unit tests are failing

Yes. I will be pushing more changes.

@TomerHeber TomerHeber merged commit 2d3ffed into main Jun 13, 2023
3 checks passed
@TomerHeber TomerHeber deleted the feat-helm-support-#655 branch June 13, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomporat247 tomporat247 tomporat247 approved these changes

@yaronya yaronya Awaiting requested review from yaronya

Assignees

@TomerHeber TomerHeber

Labels
api-client feature integration-tests provider ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

helm template support + helm repo vcs option support
2 participants
@TomerHeber @tomporat247