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

docs(registry-scanner): remove nodeSelector from docs #1337

Merged
merged 6 commits into from
Sep 11, 2023

Conversation

miketnt
Copy link
Contributor

@miketnt miketnt commented Sep 5, 2023

What this PR does / why we need it:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Title of the PR starts with type and scope, (e.g. feat(agent,node-analyzer,sysdig-deploy):)
  • Chart Version bumped for the respective charts
  • Variables are documented in the README.md (or README.tpl in some charts)
  • Check GithubAction checks (like lint) to avoid merge-check stoppers
  • All test files are added in the tests folder of their respective chart and have a "_test" suffix

Check Contribution guidelines in README.md for more insight.

@miketnt miketnt marked this pull request as ready for review September 5, 2023 13:06
@miketnt miketnt requested review from a team as code owners September 5, 2023 13:06
Copy link
Collaborator

@airadier airadier left a comment

Choose a reason for hiding this comment

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

Sorry but I disagree with the solution, not with the implementation.

The original issue was raised due to a confusion in the usage of nodeSelector. Customer was expecting the nodeSelector to apply to the worker jobs, and not only to the orchestrator.

But in current implementation, the nodeSelector applies to the "orchestrator" pod only.

The docs are not clear about this:
"Configure nodeSelector for scheduling the registry scanner pod".

I think the solution is to clarify the misunderstanding with the customer.

Then, if this is a requirement, we will need a Feature Request to implement correctly, as we did for the customLabels that now also apply to the worker jobs (the issue here was an Admission Controller with mandatory labels).

If this is not a requirement, we can just leave it as is, we can improve the documentation, or we can remove it from values.yaml and the doc. But in this case, we need to consider other parameters in the exact same situation, at least:

  • tolerations
  • affinity
  • podSecurityContext

labels and securityContext where previously addressed as part of a previous Feature Request, and now it is being properly propagated to the worker jobs.

@miketnt miketnt merged commit 7fef1a3 into master Sep 11, 2023
5 checks passed
@miketnt miketnt deleted the remove-nodeSelector-from-docs branch September 11, 2023 11:00
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.

2 participants