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.

Sign up for GitHub

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

Jump to bottom

Design document for reading status properties of other resources #2489

Merged
merged 2 commits into from
Sep 14, 2022
Merged

Design document for reading status properties of other resources #2489

merged 2 commits into from
Sep 14, 2022

Conversation

matthchr
Copy link
Member

@matthchr matthchr commented Sep 7, 2022

If applicable:

  • this PR contains documentation
  • this PR contains tests

@matthchr matthchr added this to the v2.0.0-beta.3 milestone Sep 7, 2022
@theunrepentantgeek theunrepentantgeek changed the title Design document for reading status properties of other resoruces Design document for reading status properties of other resources Sep 8, 2022
theunrepentantgeek
theunrepentantgeek approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

I've made comments reflecting all my notes; many of these overlap our discussion in the review meeting.

docs/hugo/content/design/ADR-2022-09-Reading-Status-Properties-Of-Resources.md Outdated
The idea here is that `UserManagedIdentity` would allow exporting a Secret or ConfigMap value, and then the `RoleAssignment` resource would support (optionally) sourcing from that store instead of having the value hardcoded.
The main difference between this and what we have now is for references we can't replace the entire reference with a `SecretReference`, we need a new type like `OptionalSecretReference`:

We would only opt a select few resource properties into this `OptionalSecretReference` structure. We could (via serialization magic) support new properties adopting this shape in a nonbreaking way, in case there are properties we didn't think
Copy link
Member

Choose a reason for hiding this comment

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

Task does this kind of serialization magic in a number of places - one example is around variables where they can be directly defined:

    vars:
      VAR: Hello!

or dynamically defined:

    vars:
      GIT_COMMIT:
        sh: git log -n 1 --format=%h

We can reference the Task implementation for an approach.

super-harsh
super-harsh approved these changes Sep 12, 2022
View reviewed changes
Copy link
Collaborator

@super-harsh super-harsh left a comment

Choose a reason for hiding this comment

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

LGTM. Would poll in for option 3 too.

@theunrepentantgeek theunrepentantgeek mentioned this pull request Sep 13, 2022
Open
@matthchr matthchr mentioned this pull request Sep 13, 2022
Closed
@matthchr matthchr force-pushed the feature/reading-properties-from-other-resources-doc branch from 8c36087 to 4ef1cf7 Compare September 13, 2022 18:47
@matthchr
Copy link
Member Author

/ok-to-test sha=4ef1cf7

theunrepentantgeek
theunrepentantgeek approved these changes Sep 13, 2022
View reviewed changes
@matthchr matthchr force-pushed the feature/reading-properties-from-other-resources-doc branch from 4ef1cf7 to dbf7e96 Compare September 13, 2022 23:27
@matthchr
Copy link
Member Author

/ok-to-test sha=dbf7e96

@matthchr
Copy link
Member Author

/ok-to-test sha=fac33ae

@matthchr matthchr merged commit f636e7b into Azure:main Sep 14, 2022
@matthchr matthchr deleted the feature/reading-properties-from-other-resources-doc branch September 14, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theunrepentantgeek theunrepentantgeek theunrepentantgeek approved these changes

@super-harsh super-harsh super-harsh approved these changes

@davefellows davefellows Awaiting requested review from davefellows davefellows is a code owner

@Porges Porges Awaiting requested review from Porges

@babbageclunk babbageclunk Awaiting requested review from babbageclunk babbageclunk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Azure Service Operator Roadmap
Archived in project
Milestone
v2.0.0-beta.3
Development

Successfully merging this pull request may close these issues.
3 participants
@matthchr @theunrepentantgeek @super-harsh