Skip to content

Conversation

@mboersma
Copy link
Member

@mboersma mboersma commented May 22, 2018

az acs browse should work identically to az acs kubernetes browse for an ACS Kubernetes cluster, but was broken due to a case-sensitive string comparison.

Closes #6158


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

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@promptws
Copy link

View a preview at https://prompt.ws/r/Azure/azure-cli/6405
This is an experimental preview for @microsoft users.

@mboersma mboersma changed the title [ACS] fix orchestrator type comparison in az aks browse [ACS] fix orchestrator type comparison in az acs browse May 22, 2018
@mboersma
Copy link
Member Author

mboersma commented May 22, 2018

Looks like azure-cli master is still broken. Both of my PRs failed with this:

The following modules have invalid versions:
azure-cli-core
azure-cli

Update: never mind, I see #6407.

@codecov-io
Copy link

codecov-io commented May 22, 2018

Codecov Report

Merging #6405 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #6405   +/-   ##
===================================
  Coverage    0%      0%           
===================================
  Files       11      11           
  Lines      145     145           
  Branches    11      11           
===================================
  Misses     145     145

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 2f6797e...a121f81. Read the comment docs.

@mboersma
Copy link
Member Author

Could this get reviewed soon? I'd like to get this bug fix in the next az release.

Copy link
Contributor

Choose a reason for hiding this comment

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

What type could this orchestrator_type to be? Why does it need to be cast into str first?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be an instance of azure.mgmt.containerservice.models.ContainerServiceOrchestratorTypes at runtime, which is just an enum class that can be cast to str but isn't directly comparable.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok.

@troydai troydai added this to the Sprint 39 milestone Jun 4, 2018
Copy link
Contributor

@troydai troydai left a comment

Choose a reason for hiding this comment

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

Please rebase the PR and resolve the conflict.

@mboersma mboersma force-pushed the fix-aks-browse branch 2 times, most recently from b4cc6b8 to 5a068ad Compare June 5, 2018 19:59
@mboersma
Copy link
Member Author

mboersma commented Jun 8, 2018

Given #6497 I assume I'll have to update HISTORY.rst in this PR before it can be merged.

@troydai
Copy link
Contributor

troydai commented Jun 8, 2018

Once it merge back to the dev and master I can handle that.

@troydai
Copy link
Contributor

troydai commented Jun 13, 2018

Resolve the conflict. Wait for the CI to pass.

@troydai troydai merged commit 54d6e4c into Azure:dev Jun 13, 2018
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.

4 participants