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: validating webhook #1027

Merged
merged 19 commits into from
Jan 9, 2023
Merged

docs: validating webhook #1027

merged 19 commits into from
Jan 9, 2023

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Dec 30, 2022

New validation hook to check if scale target is already managed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Relates to kedacore/keda#4001
Relates to kedacore/keda#3755
Relates to kedacore/keda#3087
Relates to kedacore/keda#3083
Relates to #1030

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer requested a review from a team as a code owner December 30, 2022 23:46
@netlify
Copy link

netlify bot commented Dec 30, 2022

Deploy Preview for keda ready!

Name Link
🔨 Latest commit 002a03c
🔍 Latest deploy log https://app.netlify.com/sites/keda/deploys/63bc09e6c4a016000893c111
😎 Deploy Preview https://deploy-preview-1027--keda.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@tomkerkhove
Copy link
Member

@JorTurFer Is this doc related to kedacore/keda#4001 (comment)?

@tomkerkhove
Copy link
Member

It looks like it

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

I did a first skim.

I think it's a good start, but would:

  1. Rename everything to "Admission Controller" which is a known concept
  2. Restructure things a bit for upcoming rules that we already have on the backlog
  3. Update the image to also use "Admission Controller"
  4. Introduce a section under "Concepts" that talks about the feature on what it is and how it helps, while the "operate" section can be a listing of how to opt-in, configure certs and give detailed overview of prevention rules

content/docs/2.10/concepts/_index.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/_index.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/prometheus.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/validating-webhook.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member Author

4. Introduce a section under "Concepts" that talks about the feature on what it is and how it helps, while the "operate" section can be a listing of how to opt-in, configure certs and give detailed overview of prevention rules

I'm not sure if this makes sense, I mean, the feature is too small and has a single responsibility, maybe in the future this takes more entity, but for the moment I wouldn't put at the same as those things

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@tomkerkhove
Copy link
Member

  1. Introduce a section under "Concepts" that talks about the feature on what it is and how it helps, while the "operate" section can be a listing of how to opt-in, configure certs and give detailed overview of prevention rules

I'm not sure if this makes sense, I mean, the feature is too small and has a single responsibility, maybe in the future this takes more entity, but for the moment I wouldn't put at the same as those things

But yet it is a core component of KEDA so we should cover it imo

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member Author

PTAL @tomkerkhove
I have moved the page from operate to concepts. TBH, I don't have enough content to have a real page if I split into concepts and operate.
For properly explaining how to use custom certs (in depth, not just the overview) I have created this issue to track it because I have to change also the cert generation in the operator/ms to use the same approach as the webhooks and I prefer to do all at once (just to clarify, I'm going to do it after finishing the webhooks feature).

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Looking very good, thank you!

Added a few suggestions, but I believe we should split the concept document and move some information to a new operate doc.

content/docs/2.10/concepts/admission-webhooks.md Outdated Show resolved Hide resolved
content/docs/2.10/concepts/admission-webhooks.md Outdated Show resolved Hide resolved
content/docs/2.10/concepts/_index.md Show resolved Hide resolved
content/docs/2.10/concepts/admission-webhooks.md Outdated Show resolved Hide resolved
content/docs/2.10/deploy.md Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

PTAL @tomkerkhove Tom Kerkhove FTE I have moved the page from operate to concepts. TBH, I don't have enough content to have a real page if I split into concepts and operate. For properly explaining how to use custom certs (in depth, not just the overview) I have created this issue to track it because I have to change also the cert generation in the operator/ms to use the same approach as the webhooks and I prefer to do all at once (just to clarify, I'm going to do it after finishing the webhooks feature).

That's OK and I like it more this way. I have made a suggestion, however, to split the pages anyway as they are targeting different audiences but I hope my example explains it better - Happy to discuss over call if it helps.

JorTurFer and others added 9 commits January 6, 2023 23:32
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, just added some small nits.

Please also update https://github.com/kedacore/keda-docs/blob/main/schematics.pptx which we should use in favor of the one in the code repo as it feels more related here than there. Thoughts? I would remove it from code repo as well.

content/docs/2.10/operate/tls-certificates.md Outdated Show resolved Hide resolved
content/docs/2.10/deploy.md Outdated Show resolved Hide resolved
content/docs/2.10/deploy.md Show resolved Hide resolved
content/docs/2.10/deploy.md Show resolved Hide resolved
content/docs/2.10/deploy.md Show resolved Hide resolved
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer
Copy link
Member Author

Done! PTAL

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge when feature is merged

content/docs/2.10/operate/cluster.md Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Please also update main/schematics.pptx which we should use in favor of the one in the code repo as it feels more related here than there. Thoughts? I would remove it from code repo as well.

@JorTurFer I think you did it the other way around :D I would keep the file here and remove it from kedacore/keda (code repo) since it is unlikely that we'll need it much there compared to here

content/docs/2.2/concepts/_index.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/security.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/security.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/security.md Outdated Show resolved Hide resolved
content/docs/2.10/operate/security.md Outdated Show resolved Hide resolved
content/docs/2.10/deploy.md Show resolved Hide resolved
@JorTurFer
Copy link
Member Author

@JorTurFer I think you did it the other way around :D I would keep the file here and remove it from kedacore/keda (code repo) since it is unlikely that we'll need it much there compared to here

that's what I did, I updated the file here and removed from keda repo

@tomkerkhove
Copy link
Member

Nevermind me; it's a 95% delete and not a complete delete - Sorry.

@JorTurFer
Copy link
Member Author

Nevermind me; it's a 95% delete and not a complete delete - Sorry.

Binary diffs xD

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@JorTurFer JorTurFer merged commit 8287884 into kedacore:main Jan 9, 2023
@JorTurFer JorTurFer deleted the validation-hook branch January 9, 2023 16:04
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.

3 participants