Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neverett , this could have been caught in flyteorg/flytesnacks#1660 if this reference didn't mention
master
as the branch, but the branch name was not used, which means that only after this was merged to master the error manifests itself.This seems to be a pattern in our docs, but I'm wondering if we shouldn't lean on a bit of automation to catch these at PR time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we do reference
master
in all the places where we use therli
directive. I'm in favor of automation to catch these at PR time, although I'm not sure what that would look like -- I can make a Linear ticket to investigate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that we had another failure (fixed in #5428). Notice how in the aforementioned PR we're trading staleness (as in changes to the original file will not be reflected to the docs since we're pointing to a specific SHA) for consistency.
@neverett , can you open a Linear ticket to discuss which approach we should take going forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eapolinario in the interest of avoiding build failures, I'll switch all
rli
URLs to SHAs for now, plus open a ticket so we can discuss how we want to approach this long term. When I started the work of separating docs from example code, the user guide examples were fairly stable, but they've seen a lot of activity in the last few weeks, and I assume will continue to be updated regularly going forward.