-
Notifications
You must be signed in to change notification settings - Fork 115
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
Rewrite packages content and split into Config and Provider chapters #566
Conversation
Signed-off-by: Pete Lumbis <[email protected]>
Signed-off-by: Pete Lumbis <[email protected]>
Signed-off-by: Pete Lumbis <[email protected]>
✅ Deploy Preview for crossplane ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Vale error is on the KB page, new content passes. |
Signed-off-by: Pete Lumbis <[email protected]>
Signed-off-by: Pete Lumbis <[email protected]>
…plane#567 Signed-off-by: Pete Lumbis <[email protected]>
this is being tracked in crossplane/crossplane#4761, which you are more than welcome to contribute to if you have the time 😄 |
The {{<hover label="meta-pkg" line="1">}}meta.pkg.crossplane.io{{</hover>}} | ||
group is for creating Provider packages. | ||
|
||
Instructions on building Providers are outside of the scope of this |
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.
is there any place where the command (or an example of it) to build a provider package is documented? it's not in this page and it's not in the linked provider development guide either...
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.
Not in this PR. I would assume a simple crossplane build provider...
cli command reference would be in a related CLI doc. #TODO
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.
Some suggestions. Removed the second instance of the revisionHistory example in the providers file since you link to the section containing the same example.
Signed-off-by: Pete Lumbis <[email protected]>
kind: Provider | ||
|
||
A _Configuration_ package is an | ||
[OCI container images](https://opencontainers.org/) containing a collection of |
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.
[OCI container images](https://opencontainers.org/) containing a collection of | |
[OCI container image](https://opencontainers.org/) containing a collection of |
settings. | ||
|
||
|
||
#### Configuration revisions |
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.
#### Configuration revisions | |
#### Revision History Limit |
I found a bit confusing not to have titles the same as the option. Given these are subsections under Installation Options
, I was expecting to see the configuration as is (e.g. Revision activation policy
instead of Upgrade policy
or Skip Dependency Resolution
instead of Ignore Dependencies
) as the title.
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.
I was conflicted on this. On one hand, I completely agree with you.
On the other hand, we will have users wanting to accomplish a task but not knowing what Crossplane term to look for (for example Revision History
) and will want to search for something more generic or task based (like "Configuration revisions").
My thought here is that if you know what you're looking for you will end up searching for "Revision History" or revisionHistoryLimit
and find the result either via search or ctrl + f on page.
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.
On the other hand, we will have users wanting to accomplish a task but not knowing what Crossplane term to look for (for example Revision History) and will want to search for something more generic or task based (like "Configuration revisions").
I would see the task as "Limiting revision history"; maybe I am biased as someone who knows internals. However, Crossplane terms are also selected with the best effort to reflect the task/configuration, so feel like they are not that different than what they are for.
Having a section talking about Configuration Revisions
(or Package Revisions
in general) makes sense to me in general, but I find it confusing to have it as a Subsection under Installation options
.
My thought here is that if you know what you're looking for you will end up searching for "Revision History" or
revisionHistoryLimit
and find the result either via search or ctrl + f on page.
Search or "ctrl + f" would work in any case, so I don't think this is something we should consider while looking for a good structure.
content/master/concepts/packages.md
Outdated
|
||
## Pushing a Package | ||
Crossplane automatically upgrades a Configuration the to the latest version | ||
available in the package cache. |
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 doesn't have to be the case and could be misleading.
Imagine the following:
- Created a configuration package with
xpkg.upbound.io/upbound/platform-ref-aws:v1
- XP downloaded
v1
to cache and installed it. - Updated
Configuration
spec.package toxpkg.upbound.io/upbound/platform-ref-aws:v2
- XP downloaded
v2
to the cache and upgraded to it with a new revision. - For some reason I decided to downgrade and change
Configuration
spec.package toxpkg.upbound.io/upbound/platform-ref-aws:v1
At this state, while both v1
and v2
are in the cache, XP would activate v1
as it is the desired state instead of automatically upgrading to the latest version available in the package cache.
Revision Activation Policy is about automatically marking the latest desired revision as active, and for Configuration packages, this doesn't have much user-facing impact in contrast to Providers where active revisions controller would process MRs. An active configuration revision would only own installed manifests (e.g. XRDs and Compositions) as controller=true
, which doesn't matter for the user.
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.
If I change the XRD APIs between v1 and v2, wouldn't downgrading lose those APIs?
I'm not sure I understand what the activation policy does in this context then.
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.
If I change the XRD APIs between v1 and v2, wouldn't downgrading lose those APIs?
Yes, but I am not sure I understood the relation with the revisionActivationPolicy here 🤔
Putting revisionActivationPolicy
aside for a second, if you have the following Configuration
, it will install v1
:
apiVersion: pkg.crossplane.io/v1
kind: Configuration
metadata:
name: platform-ref-aws
spec:
package: xpkg.upbound.io/upbound/platform-ref-aws:v1
If you change it below, it will install v2
:
apiVersion: pkg.crossplane.io/v1
kind: Configuration
metadata:
name: platform-ref-aws
spec:
package: xpkg.upbound.io/upbound/platform-ref-aws:v2
If you change it back to the previous spec, it will again install v1
. The available versions in the package cache or in the registry doesn't have any impact here, you may have v5
here or there, but the package manager will just install what you asked in the Configuration
spec. It is your responsibility to have backward-compatible upgrades or supporting rollbacks, and package manager will just do whatever it is asked to do.
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.
So, what is revisionActivationPolicy
then?
You can read more details from the design, but I couldn't find an easy answer there, so let me try to explain:
Revision Activation Policy is all about the Package reconciler automatically marking a revision as Active
or not. It doesn't impact which version is installed at any instant. spec.package
field of the package is authoritative on what version should be installed or not, as I explained above.
So the question is, what is an active revision? A revision being active has the following meanings:
- Despite all package revisions owning the CRD, XRD, or Composition installed by the package, only the active one controls it with
controller: true
in the ownerRef.- This post explains the technical details well, but, in our context, we can think it as, which revision would be responsible for acting on the instances of those resources.
- As I stated above, this doesn't have a meaning for
Configuration
packages, since no controller is running for them different thanProvider
.
- Only the controller for the
Active
revision will be running to act on the instances. For a givenProvider
withv1
andv2
revisions installed wherev2
is marked as active, the provider controller forv2
will be running and operating on the instances of the types that the provider installed, a.k.a. Managed Resources.
When revisionActivationPolicy is automatic, the package manager will simply set the desired version (i.e. spec.packagein the
Provider) as active automatically.
As an example, I installed the following:
apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
name: provider-helm
spec:
package: xpkg.upbound.io/crossplane-contrib/provider-helm:v0.14.0
revisionActivationPolicy: Automatic
There is only 1 revision, which is for v0.14.0
, and marked as Active
by package manager (since we used Automatic policy)
❯ kubectl get providerrevisions.pkg.crossplane.io
NAME HEALTHY REVISION IMAGE STATE DEP-FOUND DEP-INSTALLED AGE
provider-helm-ce18dd03e6e4 True 1 xpkg.upbound.io/crossplane-contrib/provider-helm:v0.14.0 Active 50s
If I update the spec.package
of Provider
to v0.15.0
:
❯ kubectl get providerrevisions.pkg.crossplane.io
NAME HEALTHY REVISION IMAGE STATE DEP-FOUND DEP-INSTALLED AGE
provider-helm-503c3591121b True 2 xpkg.upbound.io/crossplane-contrib/provider-helm:v0.15.0 Active 12s
provider-helm-ce18dd03e6e4 True 1 xpkg.upbound.io/crossplane-contrib/provider-helm:v0.14.0 Inactive 2m40s
Not v0.15.0
is marked as Active
automatically. Meaning, that versions controller will process my MRs and I see the provider controller is running with its version:
❯ kubectl -n crossplane-system get pods provider-helm-503c3591121b-77fb968677-p6zp9 -o yaml | grep image
image: xpkg.upbound.io/crossplane-contrib/provider-helm:v0.15.0
If I used revisionActivationPolicy: Manual
in the above flow:
apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
name: provider-helm
spec:
package: xpkg.upbound.io/crossplane-contrib/provider-helm:v0.14.0
revisionActivationPolicy: Manual
It will install v0.14.0
by creating a revision but won't mark it as Active
, so no provider pod is running at all. No one will control, hence process the MRs of this provider. (For some reason, noticing that they were not initialized as Inactive
either, but probably a small non-functional bug, we now they are not active)
❯ kubectl get providerrevisions.pkg.crossplane.io -w
NAME HEALTHY REVISION IMAGE STATE DEP-FOUND DEP-INSTALLED AGE
provider-helm-ce18dd03e6e4 True 1 xpkg.upbound.io/crossplane-contrib/provider-helm:v0.14.0 13s
❯ kubectl -n crossplane-system get pods | grep provider-helm
Similarly, if I upgrade to v0.15.0
, it will install it but still not run its controller since it will not be activated either.
We don't expect users to interact with this option much except for some advanced use cases. We only leveraged that during migration from monolith to family providers, to properly transfer ownership.
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.
@plumbis nice work 🙌
I have a concern regarding the new structure though. There are quite a lot of similar (if not duplicate) content on both Configurations
and Providers
pages now. Soon, we will need to have them under Functions
as well, and for possible future extensions (e.g. Secret Store Plugins). So, I feel like having those content in a single place but referring them from specific package pages could be a more scalable and maintainable approach.
A related example is, I recently started documenting DeploymentRuntimeConfigs
here under Providers
. They will also be relevant for Functions and I was planning to refer from Functions
to Providers
not to duplicate the same content which is not great. However, I was thinking having a single place, which is higher level to specific packages types could be a better place and was wondering whether I should move under https://docs.crossplane.io/v1.13/concepts/packages/#installing-a-package which seems to be removed with this PR.
So, my suggestion would be to keep Crossplane Packages
section and add Configurations
as a new one instead of converting the former to the latter.
@turkenh thank you for your time and feedback on this! I must have made three drafts of this going back and forth between duplicating and centralizing the content. I ended up landing on breaking it up and duplicating for a few reasons: 1.) The user workflow is going to be around a task: use a configuration, install and modify a provider, install or modify a function. 2.) The concept of a "package" is apparent if you're building a package or you're an experienced Crossplane user. If I'm trying to modify my provider it's not obvious that "package" is the right section to look under for information about my configuration options. 3.) The examples, outputs and terms are just different enough to make it confusing when it's centralized. For example, Configurations and Providers can specify different dependencies. Installing a Provider may take minutes, but installing a Configuration shouldn't. Creating a config and a provider are very different processes that just happen to use the same packaging. There are authoring options to centralize some parts and import them but this makes the authoring process a bit of a pain in the ass. Honestly I'd rather require us to be mindful of how a feature like |
Signed-off-by: Pete Lumbis <[email protected]>
I think I see your point in that a user is usually not aware that they are installing a Crossplane Package when they are installing a Configuration. However, could this be related to how the existing packages page was structured and us not doing a good job introducing the concept clearly 🤔
I see your points and agree with them. My primary concern is maintenance; once you have similar (or even the same) content in multiple places, they will eventually diverge or create hesitance for diving deep. I really wish we could find a way to avoid this. Imagine that I want to document the behavior of
The ideal structure in my mind would be something like below:
|
@plumbis @turkenh I wonder if there's some middle ground here? I came looking at this PR as I started documenting how to install a Function. I started copying what was on the Provider page, then realized if I continued down that path the reader would be scrolling through pages of detailed "how to install a package" content before they ever even saw a Composition that used a Function. I do agree there's value in repeating the really common things per package type. For example high level examples of how to install, update, and delete each package. In some cases there's extra context that is only relevant to a particular type of package - for example being wary that deleting a provider will delete any MRs it owns. However some of the detail feels ripe to deduplicate into a "package details" page. For me this would include all the conditions a package can have, and most of the installation options that are common to all packages. |
I think both of y'all have great points that should be addressed. What I would propose is that if there are no objections to the content, let's merge this. It's duplicating content but it's also a large improvement over the existing packages chapter. Sometime in the 1.15 time frame I can take on some de-dup and organizational work, is that acceptable? |
@plumbis Sounds good to me regarding figuring out the duplication later. I haven't actually reviewed this new content in any depth though so I defer to @tr0njavolta and @turkenh WRT whether it's ready to merge. Note that I can review if it would help, but my assumption is that @turkenh has already gone deep enough to potentially approve faster than waiting for me to review this. Part of #583 is going to be updating the packages docs to make sure they mention/cover Functions. I'm hopeful that won't be too hard since it should mostly be a copy of the provider docs, but I'd like to see this merged to unblock that. |
I am fine with following up for de-duplication separately. Happy to approve once the open discussions are resolved, specifically this one. |
Signed-off-by: Pete Lumbis <[email protected]>
Signed-off-by: Pete Lumbis <[email protected]>
@turkenh I simplified the language there. I can expand on it after release. Let me know if this work. |
Signed-off-by: Pete Lumbis <[email protected]>
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.
@turkenh I simplified the language there. I can expand on it after release.
Let me know if this work.
I left some comments for the technically inaccurate part.
Signed-off-by: Pete Lumbis <[email protected]>
Signed-off-by: Pete Lumbis <[email protected]>
…566) Signed-off-by: Pete Lumbis <[email protected]>
…566) Signed-off-by: Pete Lumbis <[email protected]>
…566) Signed-off-by: Pete Lumbis <[email protected]>
…566) Signed-off-by: Pete Lumbis <[email protected]>
Updates and expands the Packages chapter. The chapter is now only focused on Configuration packages.
As a result the Providers chapter has been expanded to cover package related elements in the context of Providers.
Because the title of the page changed changing the menu from
v1.13
tomaster
doesn't work for the new Packages chapter. The fix for this is in #567 and will fix these pages when merged.Packages: https://deploy-preview-566--crossplane.netlify.app/master/concepts/packages/
Providers: https://deploy-preview-566--crossplane.netlify.app/master/concepts/providers
Note: This references the new Crossplane CLI but doesn't add new docs for the CLI. We'll need to come back and add links after new CLI docs are published.
Resolves #493, #556, #454, #450, #276, #277
Signed-off-by: Pete Lumbis [email protected]