Container Instances - Use of Private IPS #90

sparsick · 2021-12-15T14:44:21Z

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

Based on @jplorier PR #60 I fixed the checkstyle issues and compilation errors.

Fixed #55
Fixes #25
Fixes #60

src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly

src/main/java/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate.java

timja

Appears to be missing setting the variable?

I would expect an update in AciService, see

azure-container-agents-plugin/src/main/java/com/microsoft/jenkins/containeragents/aci/AciService.java

Line 64 in 9ca34d6

variables.put("jenkinsInstance",

sparsick · 2021-12-16T10:21:16Z

Thanks @timja for reviewing it. I will recheck the AciService. Thanks for your hint.

timja

Couple of minor comments can you confirm you've tested this and it's working for you?

src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly

sparsick · 2021-12-17T12:28:03Z

@timja thank you for your review. I found that the config was not saved correctly (I fixed it), so my previous tests are invalid. Now I found out that the container needs a network profile. So I have to investigate the azure documentation how to do it in the best way. I think I will have to do more changes, therefore, I put this PR in draft again.

sparsick · 2021-12-23T14:05:22Z

I replaced the checkbox by an optional block with two textboxes for the needed virtual network information.

Still in progress because I have to adjust the deploy template.

sparsick · 2022-01-04T13:21:07Z

I tested the changes manually for both cases (public ip address and private ip address). A known limitation is that vnet and subnet must be in the resource group that is defined in the above field Azure Container Instance -> Resource Group

timja

It would be nice to see a Configuration as code test showing that it works there.

This is looking really close, the blockers are in the src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly file.

~~I haven't tested this myself interactively, have you?~~ I see you posted after I started my review 👍

src/main/java/com/microsoft/jenkins/containeragents/builders/AciDeploymentTemplateBuilder.java

src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly

...st/java/com/microsoft/jenkins/containeragents/builders/AciDeploymentTemplateBuilderTest.java

Review Feedback

#55: Update src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly #55: Update src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>

…oxes for virtual network information

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>

result of code review

sparsick · 2022-01-05T19:34:13Z

It would be nice to see a Configuration as code test showing that it works there.

I started with a JCasC test, but I fell in a rabbit hole because this plugin needs a dependency update. If it is OK for you, I will hand it later after a dependency update.

timja · 2022-01-05T20:25:34Z

I have a dependency update ready to go locally just need one component released, should have it up tonight / tomorrow morning UTC

timja · 2022-01-05T22:48:12Z

PR is #92, I've tested it locally and the tests pass, bom should be being released atm

timja

much better, infinite loop is the main blocker but the rest should be really quick

src/main/java/com/microsoft/jenkins/containeragents/aci/AciCleanTask.java

src/main/java/com/microsoft/jenkins/containeragents/aci/AciService.java

src/main/java/com/microsoft/jenkins/containeragents/util/CustomJenkinsFacade.java

src/main/java/com/microsoft/jenkins/containeragents/builders/AciDeploymentTemplateBuilder.java

src/main/java/com/microsoft/jenkins/containeragents/util/CustomJenkinsFacade.java

timja

non blocking comments

src/main/resources/com/microsoft/jenkins/containeragents/aci/networkProfileSnippet.json

...es/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/help-privateIpAddress.html

result of code review

sparsick · 2022-01-07T11:24:25Z

I added a minimal JCasC test for my feature. I hope that is enough for the first. Furthermore, I will open new issues for the non-blocking stuff.

timja

final nits, the configuration as code plugin is in the plugin bom so you don't need to specify a version https://github.com/jenkinsci/bom

pom.xml

src/main/java/com/microsoft/jenkins/containeragents/util/InstanceIdentityFacade.java

timja

Just to confirm you've re-tested this with the changes made during review?

I checked the UI saving etc but haven't run an actual test, all looks good though

This reverts commit 052b1a7.

sparsick · 2022-01-07T13:53:54Z

I retested it. I found that Instance Identity does not work as expected.

I rollbacked it to legacyIdentifierId this works.

oh sorry. I was too late with my answer. I will open a new PR.

sparsick · 2022-01-07T14:03:14Z

@timja New PR #95

timja · 2022-01-07T14:07:06Z

np I cancelled the release

sparsick · 2022-01-07T14:15:12Z

np I cancelled the release

Thank you 🙏

timja · 2022-01-07T14:26:34Z

https://github.com/jenkinsci/azure-container-agents-plugin/releases/tag/219.vc24a7acc8873 enjoy

timja added the enhancement label Dec 15, 2021

timja reviewed Dec 15, 2021

View reviewed changes

src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly Outdated Show resolved Hide resolved

timja reviewed Dec 15, 2021

View reviewed changes

src/main/java/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate.java Outdated Show resolved Hide resolved

timja requested changes Dec 15, 2021

View reviewed changes

sparsick marked this pull request as draft December 16, 2021 10:18

sparsick marked this pull request as ready for review December 16, 2021 11:04

sparsick requested a review from timja December 16, 2021 11:05

timja reviewed Dec 16, 2021

View reviewed changes

src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly Outdated Show resolved Hide resolved

src/main/resources/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/config.jelly Outdated Show resolved Hide resolved

sparsick force-pushed the 55-privateIps branch from 81107df to 846aafb Compare December 17, 2021 12:23

sparsick marked this pull request as draft December 17, 2021 12:24

sparsick marked this pull request as ready for review January 4, 2022 13:08

sparsick requested a review from timja January 4, 2022 13:10

timja requested changes Jan 4, 2022

View reviewed changes

sparsick force-pushed the 55-privateIps branch from e56fa02 to 56c4d1f Compare January 5, 2022 17:35

Juan Pablo Lorier and others added 12 commits January 5, 2022 18:39

55: Changes to allow selecting the IP type for the deployment

35a2fcd

#55: Fix compilation error

47ca818

#55: replace droplist by checkbox

b125ce8

Review Feedback

#55: replace adding data via contructor by setter

91047f3

Review Feedback

#55: map useIpPrivateIPAddress to IP type

f006c58

Review Feedback

#55: replace private ip address checkbox by optional block with textb…

160d213

…oxes for virtual network information

#55: refactoring as preparation for better testing

46c2903

#55: add resource networkProfile when private ip address is used

0dcf5f9

#55: improve tool tip text

9ddc589

#55: add deleting network profile to clean up tasks

7f48da5

#55: changes after code review

bb8338f

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>

#55: replace direct jenkins model call by jenkins facade

f9fe4ca

result of code review

sparsick force-pushed the 55-privateIps branch from 56c4d1f to f9fe4ca Compare January 5, 2022 19:29

sparsick requested a review from timja January 5, 2022 19:34

timja requested changes Jan 6, 2022

View reviewed changes

timja added 2 commits January 7, 2022 08:54

Merge branch 'master' into 55-privateIps

09f26bf

Fixup after master merge

cfdafe7

timja reviewed Jan 7, 2022

View reviewed changes

src/main/resources/com/microsoft/jenkins/containeragents/aci/networkProfileSnippet.json Show resolved Hide resolved

...es/com/microsoft/jenkins/containeragents/aci/AciContainerTemplate/help-privateIpAddress.html Show resolved Hide resolved

sparsick added 2 commits January 7, 2022 11:14

#55: minor changes after code review feedback

4f3642f

#55: add counter for retry deleting network profile

51cb307

result of code review

sparsick marked this pull request as draft January 7, 2022 10:25

#55: add jcasc test for the case private ip address option is set

bef7904

sparsick marked this pull request as ready for review January 7, 2022 11:23

sparsick requested a review from timja January 7, 2022 11:23

This was referenced Jan 7, 2022

Update deployment template to new API version #93

Closed

Add validation for option "use private ip address" #94

Open

timja reviewed Jan 7, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

src/main/java/com/microsoft/jenkins/containeragents/util/InstanceIdentityFacade.java Outdated Show resolved Hide resolved

#55: minor changes after code review feedback

762d7f1

sparsick requested a review from timja January 7, 2022 11:59

timja approved these changes Jan 7, 2022

View reviewed changes

timja merged commit 052b1a7 into jenkinsci:master Jan 7, 2022

sparsick added a commit to sparsick/azure-container-agents-plugin that referenced this pull request Jan 7, 2022

Revert "Container Instances - Use of Private IPS (jenkinsci#90)"

b9466a8

This reverts commit 052b1a7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Instances - Use of Private IPS #90

Container Instances - Use of Private IPS #90

sparsick commented Dec 15, 2021 •

edited by timja

Loading

timja left a comment

sparsick commented Dec 16, 2021

timja left a comment

sparsick commented Dec 17, 2021

sparsick commented Dec 23, 2021

sparsick commented Jan 4, 2022

timja left a comment •

edited

Loading

sparsick commented Jan 5, 2022

timja commented Jan 5, 2022

timja commented Jan 5, 2022

timja left a comment •

edited

Loading

timja left a comment

sparsick commented Jan 7, 2022

timja left a comment •

edited

Loading

timja left a comment

sparsick commented Jan 7, 2022

sparsick commented Jan 7, 2022

timja commented Jan 7, 2022

sparsick commented Jan 7, 2022

timja commented Jan 7, 2022

Container Instances - Use of Private IPS #90

Container Instances - Use of Private IPS #90

Conversation

sparsick commented Dec 15, 2021 • edited by timja Loading

timja left a comment

Choose a reason for hiding this comment

sparsick commented Dec 16, 2021

timja left a comment

Choose a reason for hiding this comment

sparsick commented Dec 17, 2021

sparsick commented Dec 23, 2021

sparsick commented Jan 4, 2022

timja left a comment • edited Loading

Choose a reason for hiding this comment

sparsick commented Jan 5, 2022

timja commented Jan 5, 2022

timja commented Jan 5, 2022

timja left a comment • edited Loading

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

sparsick commented Jan 7, 2022

timja left a comment • edited Loading

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

sparsick commented Jan 7, 2022

sparsick commented Jan 7, 2022

timja commented Jan 7, 2022

sparsick commented Jan 7, 2022

timja commented Jan 7, 2022

sparsick commented Dec 15, 2021 •

edited by timja

Loading

timja left a comment •

edited

Loading

timja left a comment •

edited

Loading

timja left a comment •

edited

Loading