Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NSG e2e error #3057

Merged
merged 3 commits into from
Sep 14, 2023
Merged

Fix NSG e2e error #3057

merged 3 commits into from
Sep 14, 2023

Conversation

nwnt
Copy link
Contributor

@nwnt nwnt commented Jul 24, 2023

Original discussion thread:
https://redhat-external.slack.com/archives/C03F6AA3HDH/p1689891751922919

Which issue this PR addresses:

Fixes: the spring 239 release failure, ARO-3293

It also includes the changes reverted by #3058

What this PR does / why we need it:

Fix the subtle error found during the e2e test in Dev release. The error occurred when the new NSG validate function is called for the FP before the flag alternation in checkPreconfiguredNSG called by validateSubnets. In e2e, the subnets will not have any NSG, but the new NSG validation accesses it. That's why it's giving a nil pointer reference error.

Test plan for issue:

e2e and CI are now passed.

Is there any documentation that needs to be updated for this PR?

N/A

Copy link
Collaborator

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

Can we elaborate on the issue, why it's an issue, and what this PR does to fix it?

pkg/validate/openshiftcluster_validatedynamic.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the needs-rebase branch needs a rebase label Jul 24, 2023
@github-actions
Copy link

Please rebase pull request.

Copy link
Collaborator

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

the e2e test ARO Operator - Azure Subnet Reconciler needs updating to detect if it's a preconfigured NSG cluster. If so, we should skip the e2e test for reconciling the NSG.

s-amann
s-amann previously approved these changes Aug 2, 2023
@nwnt nwnt added the next-release To be included in the next RP release rollout label Aug 3, 2023
petrkotas
petrkotas previously approved these changes Aug 4, 2023
Copy link
Member

@petrkotas petrkotas left a comment

Choose a reason for hiding this comment

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

Thanks @nwnt for the update. It seems all is addressed now.

SudoBrendan
SudoBrendan previously approved these changes Aug 8, 2023
@nwnt nwnt requested a review from bennerv August 28, 2023 02:47
@github-actions github-actions bot added the needs-rebase branch needs a rebase label Aug 28, 2023
@github-actions
Copy link

Please rebase pull request.

@nwnt nwnt dismissed stale reviews from SudoBrendan, petrkotas, and s-amann via eb9df50 September 11, 2023 05:41
@github-actions github-actions bot added needs-rebase branch needs a rebase and removed needs-rebase branch needs a rebase labels Sep 11, 2023
@github-actions
Copy link

Please rebase pull request.

@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Sep 12, 2023
Copy link
Member

@petrkotas petrkotas left a comment

Choose a reason for hiding this comment

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

@nwnt thank you for the updates, I believe all comments are addressed now.

@@ -342,6 +342,15 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
}

BeforeEach(func(ctx context.Context) {
// TODO remove this when GA
Copy link
Member

Choose a reason for hiding this comment

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

@bennerv I think this is what you were asking for.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it wasn't a remove when GA, but a "use the cluster document API field when GA"

@petrkotas
Copy link
Member

/azp run e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@petrkotas
Copy link
Member

I cleared up with @bennerv that this is good to go.

@petrkotas petrkotas merged commit 4a3ecf5 into master Sep 14, 2023
19 checks passed
@petrkotas petrkotas deleted the fix-nsg-e2e-error branch September 14, 2023 14:11
SrinivasAtmakuri pushed a commit to SrinivasAtmakuri/ARO-RP that referenced this pull request Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release To be included in the next RP release rollout ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants