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

Rewrite packages content and split into Config and Provider chapters #566

Merged
merged 14 commits into from
Oct 31, 2023

Conversation

plumbis
Copy link
Collaborator

@plumbis plumbis commented Oct 10, 2023

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 to master 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 pete@upbound.io

Signed-off-by: Pete Lumbis <pete@upbound.io>
Signed-off-by: Pete Lumbis <pete@upbound.io>
Signed-off-by: Pete Lumbis <pete@upbound.io>
@netlify
Copy link

netlify bot commented Oct 10, 2023

Deploy Preview for crossplane ready!

Name Link
🔨 Latest commit 99974b1
🔍 Latest deploy log https://app.netlify.com/sites/crossplane/deploys/65411af8b400dc000841dbe1
😎 Deploy Preview https://deploy-preview-566--crossplane.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance:
Accessibility:
Best Practices:
SEO:
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@plumbis
Copy link
Collaborator Author

plumbis commented Oct 10, 2023

Vale error is on the KB page, new content passes.

Signed-off-by: Pete Lumbis <pete@upbound.io>
Signed-off-by: Pete Lumbis <pete@upbound.io>
@jbw976
Copy link
Member

jbw976 commented Oct 11, 2023

This references the new Crossplane CLI but doesn't add new docs for the CLI.

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
Copy link
Member

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...

Copy link
Collaborator Author

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

Copy link
Contributor

@tr0njavolta tr0njavolta left a 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.

content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
Signed-off-by: Pete Lumbis <pete@upbound.io>
content/master/concepts/packages.md Outdated Show resolved Hide resolved
kind: Provider

A _Configuration_ package is an
[OCI container images](https://opencontainers.org/) containing a collection of
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[OCI container images](https://opencontainers.org/) containing a collection of
[OCI container image](https://opencontainers.org/) containing a collection of

content/master/concepts/packages.md Outdated Show resolved Hide resolved
settings.


#### Configuration revisions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### 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.

Copy link
Collaborator Author

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.

Copy link
Member

@turkenh turkenh Oct 25, 2023

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.


## Pushing a Package
Crossplane automatically upgrades a Configuration the to the latest version
available in the package cache.
Copy link
Member

@turkenh turkenh Oct 22, 2023

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:

  1. Created a configuration package with xpkg.upbound.io/upbound/platform-ref-aws:v1
  2. XP downloaded v1 to cache and installed it.
  3. Updated Configuration spec.package to xpkg.upbound.io/upbound/platform-ref-aws:v2
  4. XP downloaded v2 to the cache and upgraded to it with a new revision.
  5. For some reason I decided to downgrade and change Configuration spec.package to xpkg.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.

Copy link
Collaborator Author

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.

Copy link
Member

@turkenh turkenh Oct 25, 2023

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.

Copy link
Member

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 than Provider.
  • Only the controller for the Active revision will be running to act on the instances. For a given Provider with v1 and v2 revisions installed where v2 is marked as active, the provider controller for v2 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 theProvider) 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.

content/master/concepts/packages.md Show resolved Hide resolved
content/master/concepts/packages.md Show resolved Hide resolved
content/master/concepts/packages.md Outdated Show resolved Hide resolved
content/master/concepts/packages.md Show resolved Hide resolved
Copy link
Member

@turkenh turkenh left a 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.

content/master/concepts/providers.md Show resolved Hide resolved
@plumbis
Copy link
Collaborator Author

plumbis commented Oct 23, 2023

@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 DeploymentRuntimeConfigs is used and configured in relation to each of the respective top level objects. Your comment on Revision Activation Policy is a great example where the explanation and impact changes based on the object it's modifying.

Signed-off-by: Pete Lumbis <pete@upbound.io>
@turkenh
Copy link
Member

turkenh commented Oct 25, 2023

The concept of a "package" is apparent if you're building a package or you're an experienced Crossplane user.

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 must have made three drafts of this going back and forth between duplicating and centralizing the content. I ended up breaking it up and duplicating it for a few reasons:

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 revisionActivationPolicy I detailed above. I would never feel good about putting that amount of information in two (even 3) different places and would probably avoid diving deep in that case.

DeploymentRuntimeConfigs is another example. It could be ok to duplicate if I just put a paragraph for it. But when I want to put a good amount of details and examples to clarify the topic, it is really concerning that I need to do it in two different places and keep maintaining.

The ideal structure in my mind would be something like below:

  • Crossplane Packages
    • What is a Crossplane Package? What packages do we support today?
      • Configurations: a short description and link to the dedicated section.
      • Providers: a short description and link to the dedicated section.
      • Functions: a short description and link to the dedicated section.
    • What is a Package Revision?
      • The relation between Packages and PackageRevisions
      • What is an Active Revision
    • Installing or Upgrading a Package
    • Package Installation Options:
      • Revision History Limit
      • Revision Activation Policy
      • ...
    • Packages with Runtime: Provider and Functions are Crossplane Packages with a runtime, whereas Configuration is not.
      • DeploymentRuntimeConfig: How to configure the runtime of a package.
    • Building a Package We can move these to a developer-focused page/doc.
    • Pushing a Package We can move these to a developer-focused page/doc.
  • Configurations
    • What are Configurations?
    • Deploying a Configuration
      • Short examples on how to install
      • For more details on package installation options, see Crossplane Packages/Package Spec.
  • Providers
    • What is a Provider?
    • Deploying a Provider
      • Short examples on how to install
      • For more details on installation options, see Crossplane Packages/Package Spec.
      • Runtime Configuration, see Crossplane Packages/DeploymentRuntimeConfig
      • ProviderConfigs
  • Functions
    • What is a Function?
    • Deploying a Function
      • Short examples on how to install
      • For more details on package installation options, see Crossplane Packages/Package Spec.
      • Runtime Configuration, see Crossplane Packages/DeploymentRuntimeConfig
      • ProviderConfigs

@negz
Copy link
Member

negz commented Oct 26, 2023

I must have made three drafts of this going back and forth between duplicating and centralizing the content.

@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.

@plumbis
Copy link
Collaborator Author

plumbis commented Oct 26, 2023

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?

@negz
Copy link
Member

negz commented Oct 27, 2023

What I would propose is that if there are no objections to the content, let's merge this.

@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.

@turkenh
Copy link
Member

turkenh commented Oct 27, 2023

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.

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 <pete@upbound.io>
Signed-off-by: Pete Lumbis <pete@upbound.io>
@plumbis
Copy link
Collaborator Author

plumbis commented Oct 30, 2023

@turkenh I simplified the language there. I can expand on it after release.

Let me know if this work.

@plumbis plumbis requested a review from turkenh October 30, 2023 21:46
Signed-off-by: Pete Lumbis <pete@upbound.io>
@plumbis plumbis mentioned this pull request Oct 30, 2023
Copy link
Member

@turkenh turkenh left a 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.

content/master/concepts/providers.md Outdated Show resolved Hide resolved
content/master/concepts/providers.md Outdated Show resolved Hide resolved
content/master/concepts/providers.md Outdated Show resolved Hide resolved
Signed-off-by: Pete Lumbis <pete@upbound.io>
Signed-off-by: Pete Lumbis <pete@upbound.io>
@plumbis plumbis merged commit c0fd459 into crossplane:master Oct 31, 2023
6 of 7 checks passed
plumbis added a commit that referenced this pull request Nov 1, 2023
plumbis added a commit that referenced this pull request Nov 1, 2023
plumbis added a commit that referenced this pull request Nov 1, 2023
plumbis added a commit that referenced this pull request Nov 1, 2023
@plumbis plumbis deleted the pkg branch March 26, 2024 15:48
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.

Install Crossplane offline
5 participants