Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: validate VHD availability on azurestack #2342

Merged
merged 34 commits into from
Jan 27, 2020
Merged

feat: validate VHD availability on azurestack #2342

merged 34 commits into from
Jan 27, 2020

Conversation

deaborch
Copy link
Contributor

@deaborch deaborch commented Nov 20, 2019 •
edited
Loading

Reason for Change:

Check Azurestack for availability of VHDs

Issue Fixed:

#2076

Requirements:

Notes:
TODO:

  • Validation fixes
  • Include unit tests
  • Make feature only available on azure stack
  • Support for feature in scale
  • Support for feature in upgrade

@acs-bot acs-bot added the size/L label Nov 20, 2019
@deaborch deaborch changed the title VHD validation [WIP] Feat: Validate VHD availability on azurestack [WIP] Nov 20, 2019
@jackfrancis jackfrancis added this to the v0.45.0 milestone Nov 20, 2019
@mboersma mboersma changed the title Feat: Validate VHD availability on azurestack [WIP] feat: validate VHD availability on azurestack [WIP] Nov 21, 2019
@mboersma mboersma changed the title feat: validate VHD availability on azurestack [WIP] [WIP] feat: validate VHD availability on azurestack Nov 21, 2019
devigned
devigned suggested changes Nov 24, 2019
View reviewed changes
Copy link
Member

@devigned devigned left a comment

Choose a reason for hiding this comment

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

I think the biggest issue is additions to the client interface.

There were quite a few little nits.

cmd/deploy.go Outdated Show resolved Hide resolved
pkg/armhelpers/azurestack/vmImage.go Outdated Show resolved Hide resolved
pkg/armhelpers/azurestack/vmImage.go Outdated Show resolved Hide resolved
pkg/armhelpers/azurestack/vmImage.go Outdated Show resolved Hide resolved
pkg/armhelpers/azurestack/vmImage.go Outdated Show resolved Hide resolved
pkg/armhelpers/interfaces.go Outdated Show resolved Hide resolved
pkg/armhelpers/support-validator.go Outdated Show resolved Hide resolved
pkg/armhelpers/support-validator.go Outdated Show resolved Hide resolved
jadarsie
jadarsie reviewed Nov 26, 2019
View reviewed changes
Copy link
Member

@jadarsie jadarsie left a comment

Choose a reason for hiding this comment

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

Just a few nit comments, will continue after you push a new commit

pkg/armhelpers/vmImage.go Outdated Show resolved Hide resolved
pkg/armhelpers/vmImage.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
pkg/armhelpers/support-validator.go Outdated Show resolved Hide resolved
pkg/armhelpers/support-validator.go Outdated Show resolved Hide resolved
devigned
devigned suggested changes Nov 27, 2019
View reviewed changes
cmd/deploy.go Outdated Show resolved Hide resolved
@devigned devigned mentioned this pull request Dec 4, 2019
Closed
@codecov
Copy link

codecov bot commented Dec 5, 2019 •
edited
Loading

Codecov Report

Merging #2342 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2342      +/-   ##
==========================================
+ Coverage   71.64%   71.71%   +0.06%     
==========================================
  Files         134      134              
  Lines       24942    24942              
==========================================
+ Hits        17870    17887      +17     
+ Misses       6020     6006      -14     
+ Partials     1052     1049       -3

@acs-bot acs-bot added size/XL and removed size/L labels Dec 6, 2019
@stale stale bot removed the stale label Jan 21, 2020
@acs-bot acs-bot added size/XXL and removed size/L labels Jan 21, 2020
@acs-bot acs-bot added size/L and removed size/XXL labels Jan 21, 2020
@acs-bot acs-bot added size/XL and removed size/L labels Jan 22, 2020
@deaborch deaborch changed the title [WIP] feat: validate VHD availability on azurestack feat: validate VHD availability on azurestack Jan 27, 2020
devigned
devigned approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@devigned devigned left a comment

Choose a reason for hiding this comment

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

/lgtm

Nice work, @deaborch and @jadarsie

@acs-bot acs-bot added the lgtm label Jan 27, 2020
@acs-bot
Copy link

acs-bot commented Jan 27, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deaborch, devigned

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@acs-bot acs-bot merged commit d46d6e5 into Azure:master Jan 27, 2020
@jackfrancis jackfrancis modified the milestones: Next, v0.47.0 Jan 29, 2020
@fmotrifork fmotrifork mentioned this pull request Feb 24, 2020
Merged
@jadarsie jadarsie mentioned this pull request Nov 24, 2020
Merged
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jadarsie jadarsie jadarsie left review comments

@devigned devigned devigned approved these changes

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

Assignees

@devigned devigned

Labels
approved lgtm size/XL
Projects
None yet
Milestone
v0.47.0
Development

Successfully merging this pull request may close these issues.
5 participants
@deaborch @devigned @jadarsie @acs-bot @jackfrancis