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

Add Helm template validation to the build method in the CLI #136

Merged
merged 192 commits into from
Jan 23, 2024

Conversation

patrykkulik-microsoft
Copy link
Collaborator

@patrykkulik-microsoft patrykkulik-microsoft commented Jan 22, 2024

This PR is for [Turtle 2Wk09 (Jan 14 - Jan 27) Taskboard - Boards (azure.com)](https://dev.azure.com/msazuredev/AzureForOperators/_sprints/taskboard/Turtle/AzureForOperators/Germanium/CY24Q1/2Wk/2Wk09%20(Jan%2014%20-%20Jan%2027)?workitem=1085387)

This feature adds a pre_validate() step to the build method of the CLI which validates the helm chart used in build by running a subprocess step with the helm template command.

Changes:

  • added pre_validate() function to the build step
  • added a skip flag for the helm template step to skip the validation
  • Modified the HelmChartInput class to take in a config path instead of a dictionary. This is because I needed to use the path to the chart in the Helm chart input. I instead do the processing of the path to a dict in the HelmChartInput class.
  • in the pre_validate() function I added a separate _validate_helm_template function to perform the helm template validation for each of the charts. If there is an error in the validation I create an error file in the current working directory of the user using a jinja 2 template. I decided to do it in the current working directory because of the additional complexity that would have to be added if we were to create the folder in the build output folder or in a separate folder
  • added a function to check if the helm tool is installed
  • changed the path_to_mappings property to default_values . Functionally this property is the same, but Jacob thought it would be good to change the name and the description of it
  • I added a new error MissingDependency. I also noticed that none of our other custom exceptions inherit from errors like UserFault which is something that is recommended in the CLI. Should I change that in this PR?
  • I added a unit test - I rarely write unit tests so feedback on it welcome
  • I added a mock invalid helm chart to test functionality of my function in the unit test (This is the nf-agent-cnf-invalid folder and it doesn’t need a review

Jordan and others added 30 commits November 27, 2023 16:53
…onfig; added instructions for completing publish/delete
@Cyclam
Copy link
Collaborator

Cyclam commented Jan 22, 2024

Could you update history.rst, please? This matters now, because we need to tell Mavenir about all changes we introduce between shipments.
E.g., we've changed path_to_mappings to default_values

@Cyclam
Copy link
Collaborator

Cyclam commented Jan 22, 2024

"I also noticed that none of our other custom exceptions inherit from errors like UserFault which is something that is recommended in the CLI. Should I change that in this PR?"

  • We have a follow-on story to sort out exceptions. Could you add that point to that story, please? (Jordan should know where the story is.)

@patrykkulik-microsoft patrykkulik-microsoft merged commit bd0e37b into main Jan 23, 2024
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.

5 participants