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: updates template condition for Arc #973

Merged
merged 2 commits into from Oct 5, 2022

Conversation

nilekhc
Copy link
Contributor

@nilekhc nilekhc commented Sep 16, 2022

Signed-off-by: Nilekh Chaudhari 1626598+nilekhc@users.noreply.github.com

Reason for Change:

Requirements

  • squashed commits
  • included documentation
  • added unit tests and e2e tests (if applicable).

Issue Fixed:

fixes: #972
Does this change contain code from or inspired by another project?

  • Yes
  • No

If "Yes," did you notify that project's maintainers and provide attribution?

Special Notes for Reviewers:

@@ -22,13 +22,17 @@ spec:
{{- if .Values.linux.podAnnotations}}
{{- toYaml .Values.linux.podAnnotations | nindent 8 }}
{{- end }}
{{- if and .Values.enableArcExtension .Values.arc.enableMonitoring }}
Copy link
Member

Choose a reason for hiding this comment

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

and didn't work here?

Copy link
Contributor Author

@nilekhc nilekhc Sep 16, 2022

Choose a reason for hiding this comment

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

nope. It also tries to evaluate .Values.arc.enableMonitoring, which is not in values.yaml. It's in a separate file arc-values.yaml used while releasing ARC extension.

Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

The scan_images failure should be resolved with #976.

Were you able to repro this? IIRC, in the last PR the order in which and operands were validated?

@nilekhc
Copy link
Contributor Author

nilekhc commented Sep 16, 2022

The scan_images failure should be resolved with #976.

Were you able to repro this? IIRC, in the last PR the order in which and operands were validated?

Yeah. I remember this. But I also remember when I first implemented this I had the same issue and exactly the same was the implementation to avoid nil pointer.

@aramase
Copy link
Member

aramase commented Sep 16, 2022

The scan_images failure should be resolved with #976.
Were you able to repro this? IIRC, in the last PR the order in which and operands were validated?

Yeah. I remember this. But I also remember when I first implemented this I had the same issue and exactly the same was the implementation to avoid nil pointer.

Is this a simple repro when not using an arc cluster? Is there a way we can catch this in CI/repro it with CI so we can validate this fix and ensure we don't hit this again?

@github-actions
Copy link

github-actions bot commented Oct 1, 2022

This PR is stale because it has been open 14 days with no activity. Please comment or this will be closed in 7 days.

Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@nilekhc
Copy link
Contributor Author

nilekhc commented Oct 4, 2022

The scan_images failure should be resolved with #976.
Were you able to repro this? IIRC, in the last PR the order in which and operands were validated?

Yeah. I remember this. But I also remember when I first implemented this I had the same issue and exactly the same was the implementation to avoid nil pointer.

Is this a simple repro when not using an arc cluster? Is there a way we can catch this in CI/repro it with CI so we can validate this fix and ensure we don't hit this again?

I have added a pod annotation in the e2e tests in the helm install. So if there is an issue, CI will give an error.

@nilekhc nilekhc requested a review from aramase October 4, 2022 00:03
@nilekhc
Copy link
Contributor Author

nilekhc commented Oct 4, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilekhc
Copy link
Contributor Author

nilekhc commented Oct 4, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilekhc nilekhc merged commit ba81735 into Azure:master Oct 5, 2022
@nilekhc nilekhc deleted the pod-annotation branch October 5, 2022 17:10
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.

Helm templating fails if enableArcExtension is false and linux annotations are added
3 participants