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

Document GCP PodIdentity for PubSub #578

Merged
merged 4 commits into from
Dec 21, 2021
Merged

Document GCP PodIdentity for PubSub #578

merged 4 commits into from
Dec 21, 2021

Conversation

hermanbanken
Copy link

@hermanbanken hermanbanken commented Nov 11, 2021

Document PR kedacore/keda#2225 and issue kedacore/keda#2048.

Checklist

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

Fixes issue kedacore/keda#2048

@hermanbanken
Copy link
Author

DCO will be later... I didn't check the repo out locally yet.

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Could you please update the docs in content/docs/2.5/scalers/gcp-pub-sub.md, the PR is opened against 2.4 version which has been already released and doesn't include the change. Thanks!

content/docs/2.5/concepts/authentication.md Outdated Show resolved Hide resolved
From PR #2225 and fixes #2048

Signed-off-by: Herman Banken <[email protected]>
content/docs/2.5/scalers/gcp-pub-sub.md Outdated Show resolved Hide resolved
content/docs/2.5/scalers/gcp-pub-sub.md Outdated Show resolved Hide resolved
content/docs/2.5/scalers/gcp-pub-sub.md Outdated Show resolved Hide resolved
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.

Added a few alignments with other scalers, would you mind taking a look at them please?

content/docs/2.5/scalers/gcp-pub-sub.md Outdated Show resolved Hide resolved
content/docs/2.5/scalers/gcp-pub-sub.md Outdated Show resolved Hide resolved
@hermanbanken
Copy link
Author

hermanbanken commented Nov 16, 2021

@tomkerkhove in my opinion the interleaving of list items and (bold non-)titles and examples was confusing (see https://keda.sh/docs/2.4/scalers/gcp-pub-sub/ and https://keda.sh/docs/2.5/scalers/gcp-pub-sub/). That was why I opted for a list and then some examples. Due to the weird hierarchy people could overlook / fail to see that there are multiple authentication options.

However, maybe the sentence There are three options to configure authentication: is clear enough already.

@tomkerkhove
Copy link
Member

see keda.sh/docs/2.4/scalers/gcp-pub-sub

I'm not sure I get what you are referring to here, sorry?

@tomkerkhove in my opinion the interleaving of list items and (bold non-)titles and examples was confusing.
[...]
That was why I opted for a list and then some examples. Due to the weird hierarchy people could overlook / fail to see that there are multiple authentication options.

Unfortunately it's more about documentation consistency - If we change this then we should update all other scalers to be aligned; otherwise it will be very hard to maintain/use KEDA.

I hope that sounds reasonable to you?

So I'd suggest to align it with other scalers and open an issue to propose a change that is aligned across all scalers. Make sense?

@hermanbanken
Copy link
Author

I'm mostly complaining about the fact that the title size of the examples breaks the iteration of the different authentication parameters. I understand the consistency remark, but I do not have the time to update all existing documentation (sorry!).

Maybe There are three options to configure authentication: is a good enough heads-up that there will follow 3 different options, regardless of the messed up outline.

Screenshot 2021-11-16 at 15 39 11

@tomkerkhove
Copy link
Member

I understand the consistency remark, but I do not have the time to update all existing documentation (sorry!).

I'm definitely not asking for that :D

Maybe There are three options to configure authentication: is a good enough heads-up that there will follow 3 different options, regardless of the messed up outline.

Screenshot 2021-11-16 at 15 39 11

It's not the intent to have the samples before the other auth mechanisms though. Here is an example: https://keda.sh/docs/2.4/scalers/aws-sqs/#authentication-parameters

@zroubalik
Copy link
Member

@tomkerkhove @hermanbanken what is the status of this issue? It is one of the lastest ones for our upcoming release.

@tomkerkhove
Copy link
Member

@hermanbanken Do you have time to complete this PR or prefer if I complete it?

Signed-off-by: Herman Banken <[email protected]>
Signed-off-by: Herman Banken <[email protected]>
@hermanbanken
Copy link
Author

@tomkerkhove I've updated it to mostly comply with the AWS variant. If you think further changes are needed please feel free to make them (I wont be available the rest of the day).

@tomkerkhove tomkerkhove changed the base branch from master to gcp-workload-identity-pubsub December 21, 2021 14:54
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, merging in temp branch to fix DCO

@tomkerkhove tomkerkhove merged commit 7c1a9ea into kedacore:gcp-workload-identity-pubsub Dec 21, 2021
@hermanbanken hermanbanken deleted the docs/gcp-podidentity branch January 2, 2023 12:34
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