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

[infer] Richer value representation for custom resources #212

Open
iwahbe opened this issue Apr 7, 2024 · 2 comments
Open

[infer] Richer value representation for custom resources #212

iwahbe opened this issue Apr 7, 2024 · 2 comments
Labels
kind/enhancement Improvements or new features

Comments

@iwahbe
Copy link
Member

iwahbe commented Apr 7, 2024

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

Many infer functions operate on strongly typed values, but there is currently no way for these values to encode that a location is secret. We should enhance the surface area of types to allow for this richer information.

Related: #155

Affected area/feature
@iwahbe iwahbe added the kind/enhancement Improvements or new features label Apr 7, 2024
@iwahbe
Copy link
Member Author

iwahbe commented Apr 7, 2024

If we wanted to be really fancy (and a bit magical), we could implement pointer based lookup functions for input structs:

func IsSecret[T any](p.Context, *T) bool
func IsComputed[T any](p.Context, *T) bool

Working similarly to infer.Annotor, these functions would be used to index into a precomputed metadata table on the input type by value location. This would require that inputs are passed by reference to preserve location, and would return false for any moved value.

@iwahbe iwahbe added the 1.0.0-blocker An issue that must be resolved before 1.0.0 label Jun 22, 2024
iwahbe added a commit that referenced this issue Jul 24, 2024
@iwahbe
[BREAKING] DefaultCheck now accepts a context.Context
a47da29 
Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets
application. The new signature for `DefaultCheck` is easier to extend without additional
breaking changes. The downside to this design is that this makes `DefaultCheck` special. I
think it's worth living with that until #212 is resolved.

I'd like to merge shortly after #252 (and before #252 is released) so that user's don't
rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not
called.
@iwahbe iwahbe mentioned this issue Jul 24, 2024
Merged
iwahbe added a commit that referenced this issue Jul 26, 2024
@iwahbe
[BREAKING] DefaultCheck now accepts a context.Context
7f237dc 
Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets
application. The new signature for `DefaultCheck` is easier to extend without additional
breaking changes. The downside to this design is that this makes `DefaultCheck` special. I
think it's worth living with that until #212 is resolved.

I'd like to merge shortly after #252 (and before #252 is released) so that user's don't
rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not
called.
iwahbe added a commit that referenced this issue Jul 26, 2024
@iwahbe
[BREAKING] DefaultCheck now accepts a context.Context
371ea13 
Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets
application. The new signature for `DefaultCheck` is easier to extend without additional
breaking changes. The downside to this design is that this makes `DefaultCheck` special. I
think it's worth living with that until #212 is resolved.

I'd like to merge shortly after #252 (and before #252 is released) so that user's don't
rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not
called.
iwahbe added a commit that referenced this issue Jul 26, 2024
@iwahbe
[BREAKING] DefaultCheck now accepts a context.Context
a187b9b 
Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets
application. The new signature for `DefaultCheck` is easier to extend without additional
breaking changes. The downside to this design is that this makes `DefaultCheck` special. I
think it's worth living with that until #212 is resolved.

I'd like to merge shortly after #252 (and before #252 is released) so that user's don't
rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not
called.

Rational for the change:

Ideally, most users will not need to implement check. By design, not implementing
`CustomCheck` has the same behavior as implementing check with
`return infer.DefaultCheck(...)`. If users do implement `CustomCheck`, then we don't want
to make any assumptions about what behavior they want. This includes not applying defaults
and not injecting secrets.
iwahbe added a commit that referenced this issue Jul 27, 2024
@iwahbe
[BREAKING] DefaultCheck now accepts a context.Context
1807678 
Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets
application. The new signature for `DefaultCheck` is easier to extend without additional
breaking changes. The downside to this design is that this makes `DefaultCheck` special. I
think it's worth living with that until #212 is resolved.

I'd like to merge shortly after #252 (and before #252 is released) so that user's don't
rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not
called.

Rational for the change:

Ideally, most users will not need to implement check. By design, not implementing
`CustomCheck` has the same behavior as implementing check with
`return infer.DefaultCheck(...)`. If users do implement `CustomCheck`, then we don't want
to make any assumptions about what behavior they want. This includes not applying defaults
and not injecting secrets.
iwahbe added a commit that referenced this issue Jul 27, 2024
@iwahbe
[BREAKING] DefaultCheck now accepts a context.Context
41e6149 
Following up on #252, this allows `DefaultCheck` to safely and accurately handle secrets
application. The new signature for `DefaultCheck` is easier to extend without additional
breaking changes. The downside to this design is that this makes `DefaultCheck` special. I
think it's worth living with that until #212 is resolved.

I'd like to merge shortly after #252 (and before #252 is released) so that user's don't
rely on #252 injecting secrets when `CustomCheck` is implemented and `DefaultCheck` is not
called.

Rational for the change:

Ideally, most users will not need to implement check. By design, not implementing
`CustomCheck` has the same behavior as implementing check with
`return infer.DefaultCheck(...)`. If users do implement `CustomCheck`, then we don't want
to make any assumptions about what behavior they want. This includes not applying defaults
and not injecting secrets.
@iwahbe iwahbe removed the 1.0.0-blocker An issue that must be resolved before 1.0.0 label Sep 23, 2024
@iwahbe
Copy link
Member Author

iwahbe commented Sep 23, 2024

This is a breaking change, but I'm removing 1.0.0-blocker since I don't think we have the time to address this issue before we release 1.0.0. We can continue to iterate and release an infer/v2, to be made default in 2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/enhancement Improvements or new features
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@iwahbe