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

Add marketplace annotation to generated example manifests #75

Merged
merged 3 commits into from
Aug 29, 2022
Merged

Add marketplace annotation to generated example manifests #75

merged 3 commits into from
Aug 29, 2022

Conversation

ezgidemirel
Copy link
Member

@ezgidemirel ezgidemirel commented Aug 22, 2022

Signed-off-by: ezgidemirel ezgidemirel91@gmail.com

Description of your changes

This PR adds meta.upbound.io/example-group annotation to generated example manifests in the same file with the value of the target resource's "shortgroup/version/kind". Using this annotation, we can display these manifests together in the marketplace.

Fixes #65

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Consumed it in provider-aws and generated example manifests. An example file is below:

apiVersion: iam.aws.upbound.io/v1beta1
kind: AccessKey
metadata:
  annotations:
    meta.upbound.io/example-id: iam/v1beta1/accesskey
  labels:
    testing.upbound.io/example-name: lb
  name: lb
spec:
  forProvider:
    pgpKey: keybase:some_person_that_exists
    userSelector:
      matchLabels:
        testing.upbound.io/example-name: lb

---

apiVersion: iam.aws.upbound.io/v1beta1
kind: User
metadata:
  annotations:
    meta.upbound.io/example-id: iam/v1beta1/accesskey
  labels:
    testing.upbound.io/example-name: lb
  name: lb
spec:
  forProvider:
    path: /system/

…anifests

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>
@ezgidemirel ezgidemirel changed the title Add special annotation to generated example manifests Add marketplace annotation to generated example manifests Aug 22, 2022
defaultExampleName = "example"
defaultNamespace = "upbound-system"
labelExampleName = "testing.upbound.io/example-name"
annotationExampleGroup = "meta.upbound.io/example-group"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
annotationExampleGroup = "meta.upbound.io/example-group"
annotationExampleGroup = "meta.upbound.io/example-gvk"

As the value format for this key, my suggestion would be:

<group>/<version>.<kind>

, or

<group>/<version>/<kind>

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the PR as <shortgroup>/<version>/<kind>

Copy link
Member

@muvaf muvaf Aug 26, 2022

Choose a reason for hiding this comment

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

I don't have a strong opinion, but using the CRD name could be easier for figuring out the association on the marketplace side, i.e. <plural kind>.<group> like instances.ec2.aws.upbound.io. Version is probably irrelevant since we don't publish examples for each version.

Or maybe we can just re-use the label we use for label selectors. @hasheddan what's the best way for you folks to set up the association?

Copy link
Member

Choose a reason for hiding this comment

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

Hey folks! So I would suggest going with meta.upbound.io/example-id and then using whatever grouping value you like. The way the Marketplace is planning to handle this is to take all resources with the same meta.upbound.io/example-id, combine them into a single document to be stored, and then show them on the examples page for every resource type in the group. So, for example, if you put meta.upbound.io/example-id: super-cool-example on a VPC and 3 Subnets, those four types together would be shown as a single example on both the VPC and Subnet examples page.

Let me know if there is additional functionality that y'all feel like this does not accommodate!

Copy link
Collaborator

@ulucinar ulucinar Aug 29, 2022

Choose a reason for hiding this comment

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

Hi @hasheddan,
One concern that we currently have with this scheme (where we use the the short group name instead of a fully qualified group name, e.g., iam instead of iam.aws.upbound.io) is that if multiple providers possess groups with the same short name, would we still be able to correctly group example manifests for shared kind names?

Short group names are unique among a provider and is this enough to correctly group dependencies while showing examples? We would like to prevent a situation where an iam.Role dependency in GCP from appearing as a dependency to an AWS resource. Is the grouping done in the context of a single provider or across all providers available in the market place?

Copy link
Member

Choose a reason for hiding this comment

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

Is the grouping done in the context of a single provider or across all providers available in the market place?

@ulucinar the grouping is done in the context of a single provider package version -- so, for example, upbound/provider-aws:v0.9.0 is it's own "examples namespace" :)

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>
…ation

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @ezgidemirel, lgtm.

@ezgidemirel ezgidemirel merged commit 14881b9 into crossplane:main Aug 29, 2022
@ezgidemirel ezgidemirel deleted the example-group-annotation branch August 29, 2022 14:46
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.

Add marketplace annotation to unified example group
4 participants