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

Align scaffolding with K8up #91

Merged
merged 9 commits into from
Mar 16, 2021
Merged

Align scaffolding with K8up #91

merged 9 commits into from
Mar 16, 2021

Conversation

ccremer
Copy link
Contributor

@ccremer ccremer commented Mar 10, 2021

Summary

  • upgrades Espejo to K8s 1.20, with controller-runtime v0.8.3 and Go 1.16
  • Replaces own SyncConfigCondition with metav1.Condition
  • CI/CD:
    • Copy workflows from K8up, including changelog generator
    • CodeClimate initialized
    • Copy Makefile from K8up
  • Testing:
    • Migrate previous e2e test cases to bats-detik
    • Migrate integration tests with Ginkgo/Gomega to plain testing
  • Add apiextensions.k8s.io/v1 CRD version.
  • Change default docker user from 1000:0 to 65532:65532
  • Kustomize
    • Remove rbac-auth proxy
    • Add RBAC rules for leader election (K8s >= 1.16)
  • Update readme

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking,
    as they show up in the changelog
  • Update the documentation.
  • Update tests.

@ccremer ccremer force-pushed the scaffolding branch 4 times, most recently from 1e21969 to 85dc860 Compare March 15, 2021 16:19
@ccremer ccremer marked this pull request as ready for review March 15, 2021 16:29
@ccremer ccremer requested a review from srueg March 15, 2021 17:24
@ccremer ccremer added the change Generic change that is neither a fix or feature label Mar 15, 2021
@ccremer ccremer changed the title Update scaffolding Align scaffolding with K8up Mar 15, 2021
@@ -22,6 +14,7 @@ type (
// NamespaceSelector defines which namespaces should be targeted
NamespaceSelector *NamespaceSelector `json:"namespaceSelector,omitempty"`
// SyncItems lists items to be synced to targeted namespaces
// +kubebuilder:pruning:PreserveUnknownFields
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// +kubebuilder:pruning:PreserveUnknownFields
// +kubebuilder:pruning:PreserveUnknownFields
// +kubebuilder:validation:EmbeddedResource

Would this maybe help to get rid of the mentioned validation errors?
https://book.kubebuilder.io/reference/markers/crd-validation.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uuuh, lemme try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's having a problem with this property being an array:

The CustomResourceDefinition "syncconfigs.sync.appuio.ch" is invalid: spec.validation.openAPIV3Schema.properties[spec].properties[syncItems].type: Invalid value: "array": must be object if x-kubernetes-embedded-resource is true
		// +kubebuilder:pruning:PreserveUnknownFields
		// +kubebuilder:validation:EmbeddedResource

		// SyncItems lists items to be synced to targeted namespaces
		SyncItems []unstructured.Unstructured `json:"syncItems,omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to define a type alias for unstructured.Unstructured and set the validation on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was able to get rid of the validation error by aliasing Unstructured and put the preserveUnknownFields marker there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try with // +kubebuilder:validation:EmbeddedResource as well? Would be nice to have some basic validation that the sync items include valid K8s objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put that marker in both SyncItems []SyncItem and SyncItem. make generate did not add any other fields to the CRD spec except for preserveUnknownFields, so I left it out again.
Do you think we should do it for forward compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it in there now, even it doesn't do anything now.
// +kubebuilder:validation:EmbeddedResource may be working if Unstructured is used as its own field, but not in an array.

api/v1alpha1/syncconfig_types.go Outdated Show resolved Hide resolved
@ccremer ccremer merged commit fc68806 into master Mar 16, 2021
@ccremer ccremer deleted the scaffolding branch March 16, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Generic change that is neither a fix or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants