Consistently set and use SchemaType when exporting/importing #2428

carolynvs · 2022-10-20T20:49:18Z

What does this change

All resource files should have both a schemaVersion and a schemaType. The schemaType field serves multiple purposes:

Porter can validate it when present during the porter * apply commands to catch if the wrong file type was passed
In the future VS Code can provide autocomplete based on the schemaType and schemaVersion

In some cases resources already supported schemaType but didn't validate it or store the value in the database. It is now an official field on CredentialSetSpec, InstallationSpec, ParameterSetSpec, and the bundle Manifest (which isn't persisted to the db).

The schemaVersion for resources has been incremented and porter supports multiple versions of these resources at the same time. We will migrate to the most recent schema version when validating the resource before it's saved to the database.

I've updated our file format docs so that people can see details about a particular version of a schema, and track what changes were made between versions.

What issue does it fix

Closes #2348
Closes #2269

Notes for the reviewer

N/A

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

I wanted to use the yaml editor helper in our regular tests too, not just the smoke test framework. So I tweaked the yq editor to only take in a filesystem and not require an entire porter context to avoid a circular dependency between portercontext and yaml editor package. Signed-off-by: Carolyn Van Slyck <[email protected]>

All resource files should have both a schemaVersion and a schemaType. The schemaType field serves multiple purposes: * Porter can validate it when present during the `porter * apply` commands to catch if the wrong file type was passed * In the future VS Code can provide autocomplete based on the schemaType and schemaVersion Signed-off-by: Carolyn Van Slyck <[email protected]>

Signed-off-by: Carolyn Van Slyck <[email protected]>

VinozzZ · 2023-02-03T21:26:33Z

pkg/porter/credentials.go

 		CredentialSet: cs,
 	}
+	credSet.SchemaType = "CredentialSet"


I wonder if we can declaring a constant for each SchemaType s will help to consolidate the hard coded string into one place

VinozzZ · 2023-02-03T21:33:08Z

pkg/storage/installation.go

@@ -30,6 +31,9 @@ type Installation struct {

 // InstallationSpec contains installation fields that represent the desired state of the installation.
 type InstallationSpec struct {
+	// SchemaType indicates the type of resource imported from a file.


Do we need to have a migration step to populate values for this field for an existing database?

SchemaType is not required and supports an empty string for a value, so adding a new optional field doesn't impact existing records.

I think it's a bit confusing since the value is manually set in the code but not required in the database. If in the future some other area of the code path references the SchemaType, then it's hard to trace at what point during the code execution the value is populated. That's my main concern here

How about I populate the schemaType during apply if it's unset? Or do you only see a full db migration as the acceptable solution?

populating it during apply for installation record sounds good to me

Thanks! I really appreciate you thinking of this before I messed it up. 😊

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs · 2023-02-06T20:12:14Z

Moving back to draft while I work on bumping the schemaVersion for all resources that had schemaType added and getting a JIT migration built into Validate()

VinozzZ · 2023-02-10T15:55:44Z

pkg/storage/parameterset.go

+		// In 1.0.2 we added SchemaType and any resources with it unset should have the default value applied
+		case "1.0.1":
+			span.Info("Migrating parameter set from 1.0.1 to 1.0.2")
+			// There isn't an explicit migration for this, because schemaType is funny


I'm curious why you moved the code for setting schemaType if it's empty out of this Migrate function

Originally you were concerned about inconsistently setting the schemaType, and having a hard time understanding when it's getting set. When I moved setting the schemaType only when migrating, I realized that only creates more opportunity for inconsistency.

The migration is only triggered if their schemaVersion is on an old version, which leaves open paths for people to still end up with the schemaType unset.

For example:

I have a resource with "schemaVersion: 1.0.1" and before applying it, see that I'm on an old schemaVersion, so I update the schemaVersion to 1.0.2.

I apply the resource, and since the version is "up-to-date", the schemaType is left empty.

or

I have a resource on 1.0.1 without the schemaType set.

I apply the resource and it's auto migrated to 1.0.2 and the type is set.

I keep using the original resource file, but perhaps realize that the version is out of date, and update it to 1.0.2 but otherwise keep the same file.

I apply it again, and now the schemaType in the database is unset.

or

I have a new resource that I made by hand. I set the schemaVersion to 1.0.2 but forget to set the schemaType

I apply it and the schemaType is empty in the database (because no migration was applied).

If the concern that drove us to set the value in Validate() was consistency, only defaulting in the migration doesn't solve the consistency problem. I originally wasn't concerned about if schemaType is set in the db, which is why I left it off but was willing to add the defaulting to address the consistency concern. So we should either ensure porter sets it consistently or just not bother and go back to the original because this was a lot of changes to remain inconsistent. 😁

Thank you for the clarification!

sgettys · 2023-02-10T17:09:20Z

pkg/storage/credentialset.go

+	defer span.EndSpan()
+
+	for s.SchemaVersion != DefaultCredentialSetSchemaVersion {
+		switch s.SchemaVersion {


nit: This migration (and the other resource migration code) seems like it could get unwieldy quickly and become a bit of a maintenance headache having all of this switch logic here. Is there a way to simplify schema migration for resources using like the strategy pattern or something along those lines?

carolynvs · 2023-02-10T20:37:41Z

I'm starting to think that I'm going down the wrong path here. The change that made us think we should bump schemaVersion and do a migration is that schemaType changed from being a display only field to something persisted in the db.

I'm mostly concerned with allowing people using the operator to copy/paste the output of a porter show command into the spec of a k8s manifest. I thought having schemaType on the Spec would be helpful (I keep hoping we can eventually embed a Spec in the Operator instead of copying the fields), but I feel like it's causing a lot of trouble for not a lot of good.

Maybe I should go back to having SchemaType just on DisplayInstallation/DisplayCredentialSet/etc and validate on import but schemaType is never persisted to the db? 🤔

This PR feels like a lot considering the original request for change "throw an error when I import the wrong schemaType" was pretty small...

carolynvs · 2023-02-13T22:43:53Z

I spoke with Yingrong and we agreed that schemaVersion is a user facing compatibility guarantee. What we put in the database is an implementation detail. We should make sure that porter doesn't do anything with it's database to not be compatible with the schema but they don't have to match 1:1 at all times. In this case defaulting the schemaVersion in Validate is an implementation detail but doesn't require a schema version bump.

I will do the following:

Remove the migration part, since there's nothing to really migrate.
Rollback the schemaVersion bump but keep the nice doc changes.
Document how we decide when to bump SchemaVersion, that it's a user-facing compatibility guarantee, etc in the contributing guide.

This was missed when I originally did a sweep to isolate use of cnab-go to the cnab package. We want to avoid directly using cnab-go throughout Porter and give ourselves a single point to make adjustments or swap out types/functionality. Signed-off-by: Carolyn Van Slyck <[email protected]>

Make it possible for Porter to support a range of schemaVersion for any resource, e.g. we support Installation 1.0.2-1.0.5. Right now we don't take advantage of that but since I wrote the logic and refactored a bunch of our resources (for a change that I've since reverted) I figured it was worth keeping so that we don't need to reimplement later. Signed-off-by: Carolyn Van Slyck <[email protected]>

When the user applies a resource that does not have the schemaType set, we should default the field to the appropriate type, e.g. Installation/CredentialSpec, so that it's consistently set in the database. We don't use that field right now in Porter other than for validation but this ensures that it's consistent later when we may need to rely upon it. Signed-off-by: Carolyn Van Slyck <[email protected]>

Previously we continuously updated a single page with the most recent schema for each resource, making it hard to see the schema for a particular version. I've given each versino it's own page that calls out the changes and links directly to the json schema for that specific version. The index page will continue to show the most recent version. Signed-off-by: Carolyn Van Slyck <[email protected]>

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs · 2023-02-14T16:53:27Z

@VinozzZ @sgettys Sorry for the rebase but I needed to cleanly back out the recent changes around migration and this was easier. I've made the changes requested, can you take another look?

…er#2428) * Only use filesystem instead of a full porter context in the yq editor I wanted to use the yaml editor helper in our regular tests too, not just the smoke test framework. So I tweaked the yq editor to only take in a filesystem and not require an entire porter context to avoid a circular dependency between portercontext and yaml editor package. Signed-off-by: Carolyn Van Slyck <[email protected]> * Consistently set and validate schemaType on resources All resource files should have both a schemaVersion and a schemaType. The schemaType field serves multiple purposes: * Porter can validate it when present during the `porter * apply` commands to catch if the wrong file type was passed * In the future VS Code can provide autocomplete based on the schemaType and schemaVersion Signed-off-by: Carolyn Van Slyck <[email protected]> * Document that bundle manifests now have schemaType Signed-off-by: Carolyn Van Slyck <[email protected]> * Default SchemaType when a resource is validated Signed-off-by: Carolyn Van Slyck <[email protected]> * Alias cnab-go schema.Version to cnab.SchemaVersion This was missed when I originally did a sweep to isolate use of cnab-go to the cnab package. We want to avoid directly using cnab-go throughout Porter and give ourselves a single point to make adjustments or swap out types/functionality. Signed-off-by: Carolyn Van Slyck <[email protected]> * Keep support for multiple versions of a resource schemaVersion Make it possible for Porter to support a range of schemaVersion for any resource, e.g. we support Installation 1.0.2-1.0.5. Right now we don't take advantage of that but since I wrote the logic and refactored a bunch of our resources (for a change that I've since reverted) I figured it was worth keeping so that we don't need to reimplement later. Signed-off-by: Carolyn Van Slyck <[email protected]> * Always set schemaType before persisting a resource When the user applies a resource that does not have the schemaType set, we should default the field to the appropriate type, e.g. Installation/CredentialSpec, so that it's consistently set in the database. We don't use that field right now in Porter other than for validation but this ensures that it's consistent later when we may need to rely upon it. Signed-off-by: Carolyn Van Slyck <[email protected]> * Split out file format docs per schemaVersion Previously we continuously updated a single page with the most recent schema for each resource, making it hard to see the schema for a particular version. I've given each versino it's own page that calls out the changes and links directly to the json schema for that specific version. The index page will continue to show the most recent version. Signed-off-by: Carolyn Van Slyck <[email protected]> --------- Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs marked this pull request as ready for review October 20, 2022 21:16

carolynvs requested review from VinozzZ and vdice as code owners October 20, 2022 21:16

carolynvs marked this pull request as draft October 20, 2022 21:19

carolynvs force-pushed the schema-type-validation branch from ea694aa to 8234da8 Compare January 31, 2023 17:51

carolynvs added 2 commits January 31, 2023 11:59

carolynvs force-pushed the schema-type-validation branch from 8234da8 to e96f959 Compare January 31, 2023 19:09

Document that bundle manifests now have schemaType

78b7a85

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs marked this pull request as ready for review January 31, 2023 20:35

carolynvs requested review from bdegeeter and sgettys February 1, 2023 16:17

VinozzZ reviewed Feb 3, 2023

View reviewed changes

Default SchemaType when a resource is validated

5e1c144

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs marked this pull request as draft February 6, 2023 20:11

carolynvs force-pushed the schema-type-validation branch 2 times, most recently from 4915020 to 08d1fe1 Compare February 8, 2023 21:47

carolynvs marked this pull request as ready for review February 9, 2023 14:19

VinozzZ approved these changes Feb 10, 2023

View reviewed changes

sgettys approved these changes Feb 10, 2023

View reviewed changes

carolynvs added 4 commits February 14, 2023 09:40

carolynvs force-pushed the schema-type-validation branch from 72b696c to 0b51cbc Compare February 14, 2023 15:49

Merge branch 'main' into schema-type-validation

a28cd36

Signed-off-by: Carolyn Van Slyck <[email protected]>

VinozzZ approved these changes Feb 14, 2023

View reviewed changes

carolynvs merged commit 3946208 into getporter:main Feb 14, 2023

carolynvs deleted the schema-type-validation branch February 14, 2023 20:37

This was referenced Feb 15, 2023

Fix associated Porter version for 1.0.1 manifest schema #2580

Merged

Default bundles to schemaVersion 1.0.1 #2583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistently set and use SchemaType when exporting/importing #2428

Consistently set and use SchemaType when exporting/importing #2428

carolynvs commented Oct 20, 2022 •

edited

Loading

VinozzZ Feb 3, 2023

VinozzZ Feb 3, 2023

carolynvs Feb 3, 2023

VinozzZ Feb 6, 2023

carolynvs Feb 6, 2023

VinozzZ Feb 6, 2023

carolynvs Feb 6, 2023

carolynvs commented Feb 6, 2023

VinozzZ Feb 10, 2023

carolynvs Feb 10, 2023

VinozzZ Feb 13, 2023

sgettys Feb 10, 2023

carolynvs commented Feb 10, 2023

carolynvs commented Feb 13, 2023

carolynvs commented Feb 14, 2023

Consistently set and use SchemaType when exporting/importing #2428

Consistently set and use SchemaType when exporting/importing #2428

Conversation

carolynvs commented Oct 20, 2022 • edited Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs commented Feb 10, 2023

carolynvs commented Feb 13, 2023

carolynvs commented Feb 14, 2023

carolynvs commented Oct 20, 2022 •

edited

Loading