Skip to content

Conversation

@yinghsugn
Copy link
Contributor

@yinghsugn yinghsugn commented Feb 16, 2023

Description

  • Added optional configs (-HttpProxy, -HttpsProxy, -NoProxy, -ProxyCert) for connection behind outbound proxy server.
  • Added optional configs (-ContainerLogPath, -DisableAutoUpgrade, -NoWait, -OnboardingTimeout).
  • Fixed invalid URI issue with display name of location.
  • Fixed response can't be parsed issue with UseBasicParsing.

Checklist

  • SHOULD select appropriate branch. Cmdlets from Autorest.PowerShell should go to generation branch.
  • SHOULD make the title of PR clear and informative, and in the present imperative tense.
  • SHOULD update ChangeLog.md file(s) appropriately
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense. Add changelog in description section if PR goes into generation branch.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD have approved design review for the changes in this repository (Microsoft internal only) with following situations
    • Create new module from scratch
    • Create new resource types which are not easy to conform to Azure PowerShell Design Guidelines
    • Create new resource type which name doesn't use module name as prefix
    • Have design question before implementation
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT introduce breaking changes in Az minor release except preview version.
  • SHOULD NOT adjust version of module manually in pull request

yinghsugn and others added 8 commits February 14, 2023 12:30
Squashed commit:

[37a75ce5980] Add optional settings

[f90cd1a98ee] Add optional settings
…ashed commits)

Squashed commits:

[49e9e01caf3] Install azure-arc helm release in azure-arc-release namespace (+1 squashed commits)

Squashed commits:

[9662ecff989] Install azure-arc helm release in azure-arc-release namespace
Squashed commits:

[28ff1d7f8cf] Add Proxy settings (+4 squashed commit)

Squashed commit:

[cdfe615ad63] Update warning messages

[5d46ec26806] Check credential

[db0351e75f5] Update parameter description

[79bf2b15c29] Modify parameter type
…hed commits)

Squashed commits:

[c893de7a1b5] Fix Invalid URI issue when passing diaplsy name of location (+1 squashed commits)

Squashed commits:

[bc81f1915fb] Fix Invalid URI issue when passing diaplsy name of location
Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Please run ./build-module.ps1 to refresh docs in your local.

@BethanyZhou
Copy link
Contributor

Please run ./build-module.ps1 to refresh docs in your local.

I expected docs/New-AzConnectedKubernetes.md refreshed with syntax changes

@yinghsugn
Copy link
Contributor Author

Please run ./build-module.ps1 to refresh docs in your local.

I expected docs/New-AzConnectedKubernetes.md refreshed with syntax changes

Docs are renewed by ./build-module.ps1.


# Clear helm azure-arc environment
helm delete azure-arc --no-hooks
helm delete azure-arc -n azure-arc-release --no-hooks
Copy link
Contributor

@BethanyZhou BethanyZhou Feb 17, 2023

Choose a reason for hiding this comment

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

Have you tested your command on you local?

  • To run command on your local, please use ./run-module.ps1 after build-module.ps1
  • To record test cases, remove -Skip from It 'CreateExpanded' -skip, see L15 and I'm expecting there is a json file under test folder.

Test always is the best way to ensure quality of commands. Your test case is always skipped with -Skip Tag

Copy link
Contributor Author

@yinghsugn yinghsugn Feb 17, 2023

Choose a reason for hiding this comment

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

Thanks for pointing out this. We've tested the code with integration test and end-to-end test with the product team for GA.
I think the -Skip was there because of the test case needs pre-requisites on the environment before it started.
1.Prepare your machines for AKS Edge Essentials
2.Create a kubernetes cluster on the machine
3.Set file path for helm.exe

Copy link
Contributor

@BethanyZhou BethanyZhou Feb 17, 2023

Choose a reason for hiding this comment

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

Got it. I'd recommend preparing the environment in

if possible or make this test as -Live. -Live means this test case only can be tested in real test environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the dev and test env are separated at this moment.
Maybe we can enhance the test once the product have better integration with the dev machine.

@BethanyZhou
Copy link
Contributor

/azp run azure-powershell - security-tools

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Skip the test for the time being until the product have better integration with the dev machine.

@BethanyZhou
Copy link
Contributor

@yinghsugn could you provide change log message for upcoming release?

@BethanyZhou BethanyZhou merged commit 9db4c3b into Azure:main Feb 17, 2023
@yinghsugn yinghsugn deleted the lua/fix/connectedk8s branch February 20, 2023 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants