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

attribute access documentation #1241

Merged
merged 10 commits into from
Nov 30, 2023

Conversation

troychiu
Copy link
Member

Add the documentation for attribute access on promises.
Issue: flyteorg/flyte#4409
Associated PR: flyteorg/flyteidl#439

@ByronHsu
Copy link
Contributor

Thanks for the awesome doc! It lists out all the use cases and caveats of attribute access in an easy-to-understand way. I am wondering if moving this to "Advanced composition" makes more sense. The current content of "basic type and I/O" is more about flyte type. This is more like you want to access the promise attribute when composing a workflow. I will let @wild-endeavor and @cosmicBboy to decide

@ByronHsu
Copy link
Contributor

This pr has been pending for a while. @wild-endeavor @cosmicBboy could you help taking a look? also, @troychiu could you fix the ci errors?

cosmicBboy
cosmicBboy previously approved these changes Nov 27, 2023
Copy link
Contributor

@neverett neverett left a comment

Choose a reason for hiding this comment

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

Some small changes recommended, otherwise looks good to me!

@cosmicBboy
Copy link
Contributor

@troychiu let's incorporate @neverett 's suggestions, then we can merge this

Signed-off-by: troychiu <[email protected]>
Signed-off-by: troychiu <[email protected]>
@troychiu
Copy link
Member Author

troychiu commented Nov 30, 2023

Thank you @neverett and @cosmicBboy. I have resolved all suggestions and the CI error!

@cosmicBboy cosmicBboy merged commit aa8c72e into flyteorg:master Nov 30, 2023
92 checks passed
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.

4 participants