[DevTestLabs] Adding export-artifacts commands on formula#2707
[DevTestLabs] Adding export-artifacts commands on formula#2707tjprescott merged 1 commit intoAzure:masterfrom
Conversation
troydai
left a comment
There was a problem hiding this comment.
The changes look good to me except a few comments. Please add comments for the _export_artifacts method. For the other two comments, they are non-blocking.
There was a problem hiding this comment.
return _export_artifacts(formula=forumla) if export_artifacts else formula
There was a problem hiding this comment.
Make sure to document the size effects: because properties are deleted from each item of the artifacts list, which is not a copy of formula.formula_content.artifacts, the original sources are changed as well. This is not a problem in current usage pattern because the command returns the formula and finishes immediately. But it will construct issues when this method is reused in other settings when the formula.formula content.artifacts is reused.
There was a problem hiding this comment.
Maybe I'm late to the party. But the method name is misleading. It modifies the artifacts properties of namespace. The name validate indicate a read-only operations.
There was a problem hiding this comment.
You are a little late to the party 😁. I'm fine with this naming. It's called "validate" because it is used in the argument registration as a validator, so it is more natural when you type "validator=_validate_artifacts". It's pretty well known that validator is a terrible name since it is used for validation, transformation, etc. I would recommend sticking with the name.
tjprescott
left a comment
There was a problem hiding this comment.
Some considerations on the command design, but also please add unit tests for the validators.
There was a problem hiding this comment.
Alternatively, consider leaving the "show" operation as is and having "export-artifacts" as a convenience command. I think that would be more consistent with existing commands like this (i.e. KeyVault's get-default-policy).
Also, you could implement this by simply reflecting on the get operation and using a transformer method to massage the result (since that is really all _export_artifacts is doing). This would avoid the potential pitfall that @troydai mentions as well since it would be clear that this is an output transform only.
There was a problem hiding this comment.
Does this PR fix the fact that if you omit --artifacts it should use the formula's defaults?
There was a problem hiding this comment.
Yup _validate_artifacts method would take care of using the defaults of formula while creating vm from formula
There was a problem hiding this comment.
What's the reason for this change?
There was a problem hiding this comment.
That's a bug 🐛 Type of the shared_public_ip_address_configuration is of SubnetSharedPublicIpAddressConfiguration and not SharedPublicIpAddressConfiguration. That was discovered while manually testing out vm create.
That is because i do not have scenario tests. I've also created task to be covered soon
There was a problem hiding this comment.
You are a little late to the party 😁. I'm fine with this naming. It's called "validate" because it is used in the argument registration as a validator, so it is more natural when you type "validator=_validate_artifacts". It's pretty well known that validator is a terrible name since it is used for validation, transformation, etc. I would recommend sticking with the name.
|
@tjprescott As per you suggestion, I've kept the normal Yes i definitely need unit tests for all the validators, I'll adding all tests unit and scenario with the work item that I've created if that's fine for you. Let me know . |
|
@vishrutshah it would be much better to add the relevant unit tests now rather than count on remembering to do them later. Scenario tests, that's fine, but the unit tests should be part of this PR, particularly testing that the logic to "complete the artifact ID" is covered and protected against regression. I've seen too many instances where "name or ID" logic breaks down when not protected by unit tests (from personal experience!) |
|
@tjprescott Agreed. On it :) |
b58771b to
04a7a1d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2707 +/- ##
==========================================
+ Coverage 62.87% 62.87% +<.01%
==========================================
Files 480 480
Lines 25807 25845 +38
Branches 3905 3912 +7
==========================================
+ Hits 16226 16251 +25
- Misses 8572 8584 +12
- Partials 1009 1010 +1
Continue to review full report at Codecov.
|
bf9f1db to
34eb8e2
Compare
c8a6cf0 to
4362696
Compare
* Enable delay-load of descriptions for commands (speed up az) * Update find indexing commands to accept callables for description. * Command load time in progress * - Moved previously dead command filter from parser to application configuration. - Removed unused configuration object/argv on application create. * Remove unused argument (pylint) * Remove dummy parameter * Fix for python 2.7 * Fix yet incorrect passage of parameters * Fix up additional pylint complaints * Update tests * Update tests * Fix up more tests * Fix up more core tests * Enable delay-load of descriptions for commands (speed up az) * Update find indexing commands to accept callables for description. * [Network] Remove nulls from VPN connection show/list output (#2748) * Fix #1615. * Code review feedback. * Update test docs for running individual test and all tests in mod (#2763) * Update test docs for running individual test and all tests in mod * Made feedback changes * Make argument parameters match up. (#2717) Make lock command parameter aliases match up with resource commands. * [DevTestLabs] Adding scenario test to create simple Linux + Windows VM in lab (#2767) * WIP create linux + Windows vm in lab * Adding recording * Add some more error checking/handling. (#2768) Add more validation to resolve "lock level" for lock commands. * Fix doc references to azure.cli.commands (#2740) * Fix doc references to azure.cli.commands This module has moved to azure.cli.core.commands * Fix PyLint * Add clearer guidelines on modifying changelog (#2739) * Add clearer guidelines on modifying changelog * A few smaller changes * another small format change * Code review changes * [DevTestLabs] Exposing commands to manage secrets in the lab (#2691) * ACS Update: nulling out the windows profile so that there isn't a validation fail… (#2764) * nulling out the windows profile so that there isn't a valdiation failure for missing password ACS doesn't return a password on GET. az acs scale command does a GET then PUT, but since ACS doesn't return the password the verification is failing before the PUT is sent to ACS. There is a bug in ACS this exposes. So this shouldn't be merged until after the ACS rollout finishes. Should be about start of next week. * updating history * updating version in history * removing white space added by editor * [Compute] Fix issues with VMSS and VM availability set update. (#2773) * Fix issues with VMSS and VM availability set update. * Update help. Fix #2762. * Error out if you try to list resources for a group that doesn't exist. (#2769) * Minor text fixes (#2776) * Add docs for az lock update. (#2702) * [DevTestLab] Explicitly enable usage of saved secrets while lab vm creation (#2686) * Explicitly enable usage of saved secrets for vm creation * Better error message with not overriding competing paramters * Adding export-artifacts commands on formula (#2707) * core: apply configured defaults on optional arg (#2770) * Core:apply configured defaults on optional argument * add a test * add tests * update history doc * address review feedback * [Network] Support active-active VNet gateways (#2751) * Start active-active test scenario. * Add active-active parameter. * Active-active scenario test 1 (cross premise) * Add second active-active scenario (vnet-to-vnet) * Refine active-active gateway configuration. * Pylint... * Code review feedback * Packaged release notes and changes for 0.2.4 (#2735) * Modify HISTORY.md * Update Dockerfile * Update debian also * Add pip dependencies also * Command load time in progress * - Moved previously dead command filter from parser to application configuration. - Removed unused configuration object/argv on application create. * Remove unused argument (pylint) * Remove dummy parameter * Fix for python 2.7 * Fix yet incorrect passage of parameters * Fix up additional pylint complaints * Update tests * Update tests * Fix up more tests * Fix up more core tests * Improve load time of custom.py for profile, find and configure (speeds up raw az command) * Pylint + flake8 fixes * Fix new vm tests that failed due to perf refactoring * Update redis tests that was broken due to perf refactoring * Delay-load msrest for command executions that don't need it * Fix flake8 issues * Fixing/improving detection of pageable class * flake8 fixes * Fix broken merge from upstream/master * Fix broken merge (again) * flake8 fixes * Fix up even more merge errors from last upstream merge * Flake8 fixes (wrong number of newlines) * Fix delay load of storage assembly for az * Update history to reference improved performance
* Release aks-preview 0.4.67 * retrigger checks
fixes #2706
export-artifactsshould export artifact array from the given formulaaz lab vm createfrom formula