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

Replace InfraDef and InfraPub with XRD #1679

Merged
merged 7 commits into from
Aug 25, 2020
Merged

Replace InfraDef and InfraPub with XRD #1679

merged 7 commits into from
Aug 25, 2020

Conversation

negz
Copy link
Member

@negz negz commented Aug 20, 2020

Description of your changes

Fixes #1615
Fixes #1682
Fixes #1683

This PR merges the InfrastructureDefinition and InfrastructurePublication types into a new(ish) type; CompositeResourceDefinition, aka "XRD". An XRD is identical to an InfrastructureDefinition with the exception of its spec.claimNames field, which accepts an optional set of names that should be used to create a "composite resource claim". Composite resource claims replace the concept of a composite resource requirement - they act as a namespaced proxy for the composite resource. spec.claimNames replaces the functionality formerly offered by InfrastructurePublication. When an XRD specifies claim names it is said to "offer a resource claim", rather than "publish a resource requirement".

Each commit in this PR has a little more context on what is changing. https://github.com/crossplane/example-cnp and particularly https://github.com/crossplane/example-cnp/issues/15 contains more context on how we landed on these concepts.

I will move the PR out of draft when it has been tested and the relevant documentation has been updated.

I have:

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

How has this code been tested

I've run through all of the composition guides except for Alibaba. I don't seem to have working credentials, but I don't expect that guide to be broken given this PR only touches composition concepts.

@negz negz requested review from kasey and muvaf August 20, 2020 00:50
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

LGTM ✔️

@negz negz force-pushed the xrd branch 2 times, most recently from 5e7759b to 55960da Compare August 22, 2020 02:48
@negz
Copy link
Member Author

negz commented Aug 22, 2020

I was hoping to take this out of draft today, but I underestimated how much work it would be to test it and update the docs. I need to:

  • Update the getting started section on composition.
    • Test the GCP guide
    • Test the Azure guide
    • Test the AWS guide
    • Test the Alibaba guide
  • Update the overview, including the diagram.
  • Update the "introduction" section on composition, including the diagram.
  • Delete the composition examples - I think they're now redundant.

@negz negz force-pushed the xrd branch 3 times, most recently from 5825436 to 2df029d Compare August 25, 2020 02:52
@negz negz marked this pull request as ready for review August 25, 2020 02:52
@negz negz requested a review from hasheddan August 25, 2020 02:53
apis/apiextensions/v1alpha1/ccrd/crd.go Show resolved Hide resolved
apis/apiextensions/v1alpha1/composition_types.go Outdated Show resolved Hide resolved
docs/getting-started/run-applications.md Outdated Show resolved Hide resolved
docs/introduction/composition.md Outdated Show resolved Hide resolved
docs/introduction/composition.md Outdated Show resolved Hide resolved
docs/introduction/composition.md Outdated Show resolved Hide resolved
docs/introduction/composition.md Outdated Show resolved Hide resolved
pkg/controller/apiextensions/offered/reconciler.go Outdated Show resolved Hide resolved
Originally we had planned to have both application and infrastructure focused
composite resources. We've since changed our approach to applications, and thus
"composite resource" and "composite infrastructure resource" have become
synonymous. We tend to say "composite resource" by convention, and thus
"composite resource definition" fits as the definition of a composite resource.
We're using 'XRD' as the abbreviation to avoid confusion with CRDs.

This commit also renames InfrastructurePublication to
CompositeResourcePublication. We plan to roll this concept into the XRD rather
than a distinct resource in a future commit.

Signed-off-by: Nic Cope <negz@rk0n.org>
This commit removes the CompositeResourcePublication (XRP) type, replacing it
with a field - spec.claimNames - on a CompositeResourceDefinition (XRD). An
XRD that specifies claimNames will define both a composite resource and a
composite resource claim - a namespaced CR that platform operators may use to
offer the composite resource to their platform consumers.

As a simplification I'm currently assuming that claim names are immutable once
set. They may be added retroactively (i.e. to offer a claim for a composite that
originally did not), but they may not be updated or removed once set. We could
change this in future at the expense of some additional complexity but without
breaking API changes if we so desired.

I've also kept the claim-CRD-and-controller-wrangling code in its own controller
in part because I prefer two quite complex reconcile loops to one really complex
reconcile loop, and in part because it was a smaller change to the current code
to do so.

Signed-off-by: Nic Cope <negz@rk0n.org>
Signed-off-by: Nic Cope <negz@rk0n.org>
Previously we omitted this information, assuming it was easily inferrable due
to the fact that a requirement for composite kind Foo was always of kind
FooRequirement. This is no longer the case.

Signed-off-by: Nic Cope <negz@rk0n.org>
I don't think these are worth the effort to maintain. We have extensive examples
in the docs, including snippets that can easily be applied, and we're starting
work on an even more extensive example repository.

Signed-off-by: Nic Cope <negz@rk0n.org>
This documentation update should capture:

* The migration from InfraDef and InfraPub to XRD
* The switch from 'requirement' to 'claim'
* The removal of the concept of 'publishing' infrastructure

Signed-off-by: Nic Cope <negz@rk0n.org>
@negz negz merged commit f4d04eb into crossplane:master Aug 25, 2020
@hasheddan hasheddan mentioned this pull request Aug 26, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants