Skip to content

Fix (#2644) - Add json tags to InfrastructureRole struct #2645

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

Merged
merged 2 commits into from
May 31, 2024

Conversation

hemakshis
Copy link
Contributor

@hemakshis hemakshis commented May 30, 2024

Fixes: #2644

@FxKu
Copy link
Member

FxKu commented May 31, 2024

Thanks. All the other mappings in this config file use name:... instead of json:.... Can you test if it also works with name?

EDIT: I think you're correct here with using json. Looks like this option is not supported by ConfigMap configuration anyway as the mapping is "-". Thanks for this finding.

re-EDIT: Actually we do use this option also via configmap with such a configuration: secretname:my-infrastructure-secret,userkey:myuser,passwordkey:mypw,defaultrolevalue:robot. So we have to check out first if it would continue to work with this JSON mapping.

@FxKu FxKu added this to the 1.12.0 milestone May 31, 2024
@FxKu
Copy link
Member

FxKu commented May 31, 2024

I feel we could also more tests and docs on this topic. But for now we can merge as is to include it into today's release.

@hemakshis
Copy link
Contributor Author

It is working after adding name tag as well, do you want me to revert it? 😅

@hemakshis
Copy link
Contributor Author

I feel we could also more tests and docs on this topic. But for now we can merge as is to include it into today's release.

Yes, docs are very confusing and each doc file gives some extra information which is not present in another. It would be good to make it consistent for all the ways we can provide these infra roles - CRD, ConfigMap and then both Only secrets & both Secret+ConfigMap.

@FxKu
Copy link
Member

FxKu commented May 31, 2024

👍

1 similar comment
@idanovinda
Copy link
Member

👍

@FxKu FxKu merged commit 34f9cfb into zalando:master May 31, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

infrastructure_role_secrets field doesn't work as expected.
4 participants