Skip to content
This repository has been archived by the owner on Jul 4, 2024. It is now read-only.

Added resource annotations and parameters support #39

Merged
merged 5 commits into from
May 23, 2023
Merged

Conversation

ghen
Copy link
Contributor

@ghen ghen commented Apr 18, 2023

Adds support for resource annotations (see score-spec/spec#56) and resource inputs hints (see score-spec/spec#9).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • New chore (expected functionality to be implemented)

Checklist:

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I've signed off with an email address that matches the commit author.

Signed-off-by: Eugene Yarshevich <yarshevich@gmail.com>
Signed-off-by: Eugene Yarshevich <yarshevich@gmail.com>
Signed-off-by: Eugene Yarshevich <yarshevich@gmail.com>
Copy link
Contributor

@chrishumanitec chrishumanitec left a comment

Choose a reason for hiding this comment

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

There is a missing case for referencing private workload resources in another workload.

internal/humanitec/convert.go Show resolved Hide resolved
internal/humanitec/convert.go Outdated Show resolved Hide resolved
}
// END (DEPRECATED)

if strings.HasPrefix(resId, "externals.") {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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:

modules.{workloadId}.externals.{resId}

In this case, there are 2 outcomes:

  1. if {workloadId} matches the current workload, treat as externals.{resId}
  2. if {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 to externals or shared properties.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

@chrishumanitec chrishumanitec May 2, 2023

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?
The workload ID is known because the delta includes it. This is the workload ID to consider.

Here are the formats to support:

  1. shared.{resId} -> replace
  2. externals.{resId} -> treat at externals.{resId}
  3. modules.{workloadId}.externals.{redId} -> replace. If workloadId == current workload ID (as determined when creating the delta treat at externals.{resId} otherwise replace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Eugene Yarshevich <yarshevich@gmail.com>
…tions

Signed-off-by: Eugene Yarshevich <yarshevich@gmail.com>
Copy link
Contributor

@chrishumanitec chrishumanitec left a comment

Choose a reason for hiding this comment

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

LGTM

@ghen ghen merged commit 5619629 into main May 23, 2023
@ghen ghen deleted the feature/metadata branch May 23, 2023 07:49
@rachfop rachfop mentioned this pull request Jul 10, 2023
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants