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

Deprecate v1alpha1 #524

Merged
merged 8 commits into from
Jul 13, 2021
Merged

Deprecate v1alpha1 #524

merged 8 commits into from
Jul 13, 2021

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Jul 5, 2021

What does this PR do?

The purpose of this PR is deprecating v1alpha1 but since the current controller-tools does not support it, it also upgrades it to v0.6.1 the latest where it's available kubernetes-sigs/controller-tools@2fc390d

What issues does this PR fix or reference?

It's part of devfile/devworkspace-operator#471, other part will be on DWO side

Is your PR tested? Consider putting some instruction how to test your changes

It's tested with installing DWO and making sure everything works as expected. More see the corresponding section in devfile/devworkspace-operator#478

Docs PR

generator/validate/gen.go Outdated Show resolved Hide resolved
schemas/latest/dev-workspace-template-spec.json Outdated Show resolved Hide resolved
@sleshchenko sleshchenko force-pushed the deprecateV1alpha1 branch 4 times, most recently from 370c7e7 to aa1d0f9 Compare July 6, 2021 12:56
@sleshchenko sleshchenko requested review from amisevsk and JPinkney July 6, 2021 12:59
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @davidfestal's review as well, as he's more familiar with the generator code.

generator/crds/gen.go Outdated Show resolved Hide resolved
generator/schemas/gen.go Outdated Show resolved Hide resolved
generator/schemas/gen.go Outdated Show resolved Hide resolved
@@ -81,7 +81,8 @@
},
"fieldsV1": {
"description": "FieldsV1 holds the first JSON version format as described in the \"FieldsV1\" type.",
"type": "Any"
"type": "object",
"additionalProperties": false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it expected to have an object with no properties and additionalproperties to false? That seems strange because if I'm not mistaken it just prevents putting anything in the corresponding field. Maybe this should be managed as a special case (maganedFields is somehow a special case in itself...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!
It was tricky but I found a workaround 20a4516

But I think additional work is needed out of the current PR scope since(the main branch state) we generate all metadata additionalProperties: false, like managedField https://github.com/devfile/api/blob/main/schemas/latest/dev-workspace-template.json#L100
But K8s Json Schema does not specify any and AFAIU it's supposed to be true by default https://kubernetesjsonschema.dev/v1.14.0/_definitions.json#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta
Screenshot_20210709_140215

@sleshchenko
Copy link
Member Author

After discussion with @davidfestal I dropped adding metadata since seems I just misunderstood the referenced PR and it will lead to issues.

Copy link
Collaborator

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

Great !

Do you think you could open an issue about ensuring, in the validate generator, that all Attributes fields have the expected 3 annotations defined (cf PR #535) ?

@openshift-ci
Copy link

openshift-ci bot commented Jul 13, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, davidfestal, sleshchenko

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link

openshift-ci bot commented Jul 13, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm label Jul 13, 2021
@sleshchenko
Copy link
Member Author

pushed with force where I just reorganize commits to have clear history

@sleshchenko sleshchenko merged commit 03e023e into main Jul 13, 2021
@sleshchenko sleshchenko deleted the deprecateV1alpha1 branch July 13, 2021 12: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.

3 participants