Add build processors #128
Add build processors #128
Conversation
* Initial ArmInputTemplate class * Outline ArmBuildProcessor hierarchy * Update VNF tests to keep built output for comparison * Initial ArmInputTemplate class * Outline ArmBuildProcessor hierarchy * Update VNF tests to keep built output for comparison * WIP, for sharing with Jacob * Incremental update * Approximately complete ARM processor. No tests, no testing yet. * Fix artifact store reference --------- Co-authored-by: Andy Churchard <andy.churchard@metaswitch.com>
| "publisherName": publisher_name, | ||
| "nfdgName": nfdg_name, | ||
| "nfdvName": self.network_function_definition.name, | ||
| "publisherResourceGroup": publisher_resource_group, |
There was a problem hiding this comment.
publisher_name, nfdg_name and publisher_resource_group are only defined if line 54 (if self.network_function_definition.id:) is True.
What happens if line 54 is false? We'll get an undefined variable error, right?
| :return: The resource element template. | ||
| :rtype: NFDResourceElementTemplate | ||
| """ | ||
| logger.info("Generating resource element template for NFD input.") |
There was a problem hiding this comment.
| logger.info("Generating resource element template for NFD input.") | |
| logger.info("Generating resource element template for %s NFD input.", self.name ) |
| logger.error("VHDFileInput must have either a file path or a blob SAS URI.") | ||
| raise ValueError( | ||
| "VHDFileInput must have either a file path or a blob SAS URI." | ||
| ) |
There was a problem hiding this comment.
This feels like the wrong place to be doing this validation. It should have been validated before it gets to the processors.
If this is duplicate validation, let's delete it. If it hasn't been validated before this (or we don't know), let's leave it as is for now, but add to a todo list (probably related to Pydantic).
|
|
||
| for file in template_dir.iterdir(): | ||
| # Template files are only ever YAML files. | ||
| if file.name.endswith(".yaml"): |
There was a problem hiding this comment.
Do we need to include .yml files?
|
|
||
| def _find_image_pull_secrets_values_paths( | ||
| self, chart: HelmChartInput, matches: Set[str] | ||
| ) -> None: |
There was a problem hiding this comment.
This is another slightly unusual method that doesn't return anything, nor update an instance variable, so presumably directly updates a variable passed by reference.
If so, I think that should be in the doc string, and possibly in the places that call this method too, as it's really unusual in our code.
There was a problem hiding this comment.
I'm not convinced this method does anything useful!
It doesn't return anything.
It doesn't set any parameters on the instance (the only instance of self is the recursive call to self._find_image_pull_secrets_values_paths() .
In the end, we just delete imagePullSecrets (because NFM supplies them), and that removal method doesn't worry at all about all this recursive searching. I think this can all be deleted.
|
This has useful review comments that we should do, not closing this PR until that. Although, the changes are already in main so this will never be completed |
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.