This repository has been archived by the owner on Jul 4, 2024. It is now read-only.
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.
Added resource annotations and parameters support #39
Added resource annotations and parameters support #39
Changes from 3 commits
80f216c
0f98c26
af6aebe
a4de2fc
31ce21a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is also possible to reference resources in other workloads - e.g.
modules.other-workload.externals.my-res
This case would fail here.
This does not require explicit addition of a resource to the delta, but it should be allowed.
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.
We are adding a new feature here. This logic was not supported with the extensions file mechanism.
Let's create a separate issue and describe possible cases, requirements, and app behaviour there.
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.
This third case is as follows:
the res-id can have the format:
In this case, there are 2 outcomes:
{workloadId}
matches the current workload, treat asexternals.{resId}
{workloadId}
does not match the current workload, just re-render the placeholder as having the value of the annotation (i.e. don't add anything toexternals
orshared
properties.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.
workload-id
is unknown to score. what do we compare with?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.
Are we to support short versions if the same res-id? E.g.
external.{resId}
? What about app-shared res-ids? How would they look like?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.
Here are the formats to support:
shared.{resId}
-> replaceexternals.{resId}
-> treat atexternals.{resId}
modules.{workloadId}.externals.{redId}
-> replace. IfworkloadId
== current workload ID (as determined when creating the delta treat atexternals.{resId}
otherwise replace.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.
Fixed