Skip to content

[Network] Make DNS Zone record imports relative#2825

Merged
tjprescott merged 2 commits intoAzure:masterfrom
tjprescott:FixDnsZoneImport
Apr 11, 2017
Merged

[Network] Make DNS Zone record imports relative#2825
tjprescott merged 2 commits intoAzure:masterfrom
tjprescott:FixDnsZoneImport

Conversation

@tjprescott
Copy link
Copy Markdown
Member

@tjprescott tjprescott commented Apr 11, 2017

Fixes #2752. Note that issue #2824 remains and can only be addressed by a service side fix.

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

  • Each command and parameter has a meaningful description.
  • Each new command has a test.

(see Authoring Command Modules)

@tjprescott tjprescott added bug This issue requires a change to an existing behavior in the product in order to be resolved. Network az network vnet/lb/nic/dns/etc... labels Apr 11, 2017
@tjprescott tjprescott added this to the Sprint 15 milestone Apr 11, 2017
@tjprescott tjprescott requested a review from derekbekoe April 11, 2017 17:37
@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 11, 2017 •
edited
Loading

Codecov Report

Merging #2825 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2825      +/-   ##
==========================================
- Coverage   62.88%   62.88%   -0.01%     
==========================================
  Files         464      464              
  Lines       25884    25890       +6     
  Branches     3942     3944       +2     
==========================================
+ Hits        16278    16281       +3     
- Misses       8588     8590       +2     
- Partials     1018     1019       +1
Impacted Files Coverage Δ
...etwork/azure/cli/command_modules/network/custom.py 62.37% <50%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b1b4e...cf74521. Read the comment docs.

derekbekoe
derekbekoe reviewed Apr 11, 2017
View reviewed changes
src/command_modules/azure-cli-network/tests/test_network_commands.py Outdated
Copy link
Copy Markdown
Member

@derekbekoe derekbekoe Apr 11, 2017

Choose a reason for hiding this comment

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

nit: debug true here.

Copy link
Copy Markdown
Member Author

@tjprescott tjprescott Apr 11, 2017

Choose a reason for hiding this comment

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

Noooooooooooooooooo! ☹️

Copy link
Copy Markdown
Member Author

@tjprescott tjprescott Apr 11, 2017

Choose a reason for hiding this comment

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

Removed. 🕺

src/command_modules/azure-cli-network/azure/cli/command_modules/network/custom.py Outdated
Copy link
Copy Markdown
Member

@derekbekoe derekbekoe Apr 11, 2017

Choose a reason for hiding this comment

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

what's -(len(origin) + 2) for?

Copy link
Copy Markdown
Member Author

@tjprescott tjprescott Apr 11, 2017

Choose a reason for hiding this comment

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

Part of the workaround to strip the origin back off of the name. The old implementation was to use absolute names. If/when the service fixes itself to allow this, I will just remove this workaround and import all records as absolute references (which the DNS import spec says it should support).

@tjprescott tjprescott merged commit 4769d93 into Azure:master Apr 11, 2017
@tjprescott tjprescott deleted the FixDnsZoneImport branch April 11, 2017 19:40
@tjprescott tjprescott mentioned this pull request Apr 19, 2017
Closed
@haroldrandom haroldrandom added bug This issue requires a change to an existing behavior in the product in order to be resolved. cla-not-required Network az network vnet/lb/nic/dns/etc... labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@derekbekoe derekbekoe derekbekoe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug This issue requires a change to an existing behavior in the product in order to be resolved. cla-not-required Network az network vnet/lb/nic/dns/etc...

Projects

None yet

Milestone

Sprint 15

Development

Successfully merging this pull request may close these issues.

5 participants

@tjprescott @codecov-io @derekbekoe @msftclas @haroldrandom