Fix DeleteOptions on Public Ip Address (issue#38806) #38987

v-hongli1 · 2024-02-28T09:14:16Z

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

azure-sdk · 2024-02-28T09:23:58Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

com.azure.resourcemanager:azure-resourcemanager-compute
com.azure.resourcemanager:azure-resourcemanager-network

weidongxu-microsoft

There is too many issue in the PR.

Please book a meeting next week to discuss.

weidongxu-microsoft · 2024-03-01T07:29:50Z

...ork/src/main/java/com/azure/resourcemanager/network/implementation/NetworkInterfaceImpl.java

+        for (NicIpConfiguration ipConfiguration : this.nicIPConfigurations.values()) {
+            if (ipConfigName.equals(ipConfiguration.name())) {
+                specifiedIPConfig = (NicIpConfigurationImpl) ipConfiguration;
+                break;
+            }
+        }


This part is just ipConfigurations().get(ipConfigName)?

weidongxu-microsoft · 2024-03-01T07:30:38Z

...ork/src/main/java/com/azure/resourcemanager/network/implementation/NetworkInterfaceImpl.java

+        if (specifiedIPConfig == null) {
+            specifiedIPConfig = prepareNewNicIPConfiguration(ipConfigName);
+            withIPConfiguration(specifiedIPConfig);
+        }


No, we don't want to modify anything in a getter API.

XiaofeiCao · 2024-03-04T05:19:34Z

...manager-network/src/main/java/com/azure/resourcemanager/network/models/NetworkInterface.java

+     * @param ipConfigName the name of th specified ip configuration
+     * @return the specified IP configuration of this network interface
+     */
+    NicIpConfiguration specifiedIPConfiguration(String ipConfigName);


This is equivalent to ipConfigurations().get(ipoConfigName)?

XiaofeiCao · 2024-03-04T05:29:27Z

...ork/src/main/java/com/azure/resourcemanager/network/implementation/NetworkInterfaceImpl.java

+        DeleteOptions deleteOptions, PublicIpAddress publicIpAddress, String ipConfigName) {
+        this.specifiedIPConfiguration(ipConfigName).withExistingPublicIpAddress(publicIpAddress);
+        this.specifiedIPConfiguration(ipConfigName).withPrivateIpVersion(publicIpAddress.version());
+        this.publicIPAddressDeleteOption = deleteOptions;
+        return this;


The implementation seems to differ from the interface declaration?

In interface, it's Update delete option for specified public ip address of specified network interface ip config, but here it seems to attach an existing PublicIpAddress to the NetworkInterface.

XiaofeiCao · 2024-03-04T05:33:25Z

...cemanager-compute/src/main/java/com/azure/resourcemanager/compute/models/VirtualMachine.java

+            /**
+             * Update delete option for the primary public ip address of the primary network interface.
+             *
+             * @param deleteOptions what happens to the public IP address when the VM using it is deleted
+             * @return the next stage of the update
+             */
+            Update withPrimaryNicPrimaryPipDeleteOption(DeleteOptions deleteOptions);
+
+            /**
+             * Update delete option for the primary public ip address of the specified network interface.
+             *
+             * @param deleteOptions what happens to the public IP address when the VM using it is deleted
+             * @param nicIds resource Ids of the specified network interface
+             * @return the next stage of the update
+             */
+            Update withSpecifiedNicPrimaryPIpDeleteOption(DeleteOptions deleteOptions, String... nicIds);
+
+            /**
+             * Update delete option for the specified public ip address of the specified network interface.
+             *
+             * @param deleteOptions what happens to the public IP address when the VM using it is deleted
+             * @param nicId resource id of the specified network interfac
+             * @param ipConfigIds resource Ids of specified network interface ip configs
+             * @return the next stage of the update
+             */
+            Update withSpecifiedNicSpecifiedPIpDeleteOption(DeleteOptions deleteOptions, String nicId, String... ipConfigIds);


Not sure whether user really needs these APIs.

IMHO, two types are needed:

withPublicIPAddressDeleteOptions(DeleteOptions deleteOptions) to update all attached PublicIpAddress'es deleteOption

withPublicIPAddressDeleteOption(PublicIpAddress(/String) publicIpAddress(id)), like what user specifies in the creation flow.

XiaofeiCao · 2024-03-04T05:34:11Z

...r-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java

+            .withNewResourceGroup(rgName)
+            .withNewPrimaryNetwork("10.0.0.0/28")
+            .withPrimaryPrivateIPAddressDynamic()
+            .withExistingPrimaryPublicIPAddress(publicIPAddress)


Should we do .withExistingPrimaryPublicIPAddress(publicIPAddress, deleteOptions)?

XiaofeiCao · 2024-03-04T05:34:32Z

...r-compute/src/test/java/com/azure/resourcemanager/compute/VirtualMachineOperationsTests.java

+            .withAdminPassword(password())
+            .withNewDataDisk(127)
+            .withSize(VirtualMachineSizeTypes.fromString("Standard_D2a_v4"))
+            .withPublicIPAddressDeleteOptions(DeleteOptions.DELETE)


Not sure whether we should use this in creation flow. I think it's for update flow.

XiaofeiCao · 2024-03-04T05:50:51Z

...network/src/test/java/com/azure/resourcemanager/network/NetworkInterfaceOperationsTests.java

+            .withPrivateIpAddressDynamic()
+            .withExistingPublicIpAddress(publicIPAddress2)
+            .attach()
+            .withPublicIPAddressDeleteOptions(DeleteOptions.DELETE)


Update flow.

XiaofeiCao · 2024-03-04T05:51:58Z

...network/src/test/java/com/azure/resourcemanager/network/NetworkInterfaceOperationsTests.java

+            .withExistingPrimaryNetwork(vnet)
+            .withSubnet(subnetName)
+            .withPrimaryPrivateIPAddressDynamic()
+            .withExistingPrimaryPublicIPAddress(publicIPAddress1)


withExistingPrimaryPublicIPAddress(publicIPAddress1, deleteOption)?

XiaofeiCao · 2024-03-04T05:53:55Z

...network/src/test/java/com/azure/resourcemanager/network/NetworkInterfaceOperationsTests.java

+            .withExistingNetwork(vnet)
+            .withSubnet(subnetName)
+            .withPrivateIpAddressDynamic()
+            .withExistingPublicIpAddress(publicIPAddress2)


withExistingPublicIpAddress(publicIPAddress2, deleteOption)?

XiaofeiCao · 2024-03-04T05:56:32Z

...manager-network/src/main/java/com/azure/resourcemanager/network/models/NetworkInterface.java

+             * @param deleteOptions the delete options for primary network interfaces
+             * @return the next stage of the definition
+             */
+            WithCreate withPublicIPAddressDeleteOptions(DeleteOptions deleteOptions);


Update flow?

XiaofeiCao · 2024-03-04T05:57:35Z

...manager-network/src/main/java/com/azure/resourcemanager/network/models/NetworkInterface.java

+             * @param ipConfigName the name of specified network interface ip config
+             * @return the next stage of the update
+             */
+            Update withSpecifiedNicSpecifiedPipDeleteOption(DeleteOptions deleteOptions, PublicIpAddress publicIpAddress, String ipConfigName);


`withPublicIpAddressDeleteOption(PublicIpAddress pip, DeleteOptions deleteOption)?

Fix DeleteOptions on Public Ip Address (issue#38806)

e6bab14

github-actions bot added the Mgmt This issue is related to a management-plane library. label Feb 28, 2024

v-hongli1 and others added 3 commits February 28, 2024 17:46

Fix DeleteOptions on Public Ip Address (issue#38806)

5e7ba1f

Merge branch 'main' into issue#38806

ff4d1df

Fix DeleteOptions on Public Ip Address (issue#38806)

2b2b6b8

v-hongli1 marked this pull request as ready for review February 29, 2024 06:52

v-hongli1 requested review from weidongxu-microsoft, haolingdong-msft, XiaofeiCao and ArthurMa1978 as code owners February 29, 2024 06:52

weidongxu-microsoft reviewed Mar 1, 2024

View reviewed changes

XiaofeiCao reviewed Mar 4, 2024

View reviewed changes

v-hongli1 closed this by deleting the head repository Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DeleteOptions on Public Ip Address (issue#38806) #38987

Fix DeleteOptions on Public Ip Address (issue#38806) #38987

v-hongli1 commented Feb 28, 2024 •

edited

Loading

azure-sdk commented Feb 28, 2024 •

edited

Loading

weidongxu-microsoft left a comment

weidongxu-microsoft Mar 1, 2024

weidongxu-microsoft Mar 1, 2024

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024 •

edited

Loading

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024 •

edited

Loading

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024

XiaofeiCao Mar 4, 2024

Fix DeleteOptions on Public Ip Address (issue#38806) #38987

Fix DeleteOptions on Public Ip Address (issue#38806) #38987

Conversation

v-hongli1 commented Feb 28, 2024 • edited Loading

Description

All SDK Contribution checklist:

azure-sdk commented Feb 28, 2024 • edited Loading

weidongxu-microsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiaofeiCao Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiaofeiCao Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-hongli1 commented Feb 28, 2024 •

edited

Loading

azure-sdk commented Feb 28, 2024 •

edited

Loading

XiaofeiCao Mar 4, 2024 •

edited

Loading

XiaofeiCao Mar 4, 2024 •

edited

Loading