Skip to content

doc/user/migrating-existing-apis.md: document API version migra… #1680

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.

Sign up for GitHub

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

Jump to bottom
Merged
estroz merged 9 commits into from
Sep 26, 2019
Merged

doc/user/migrating-existing-apis.md: document API version migra… #1680

estroz merged 9 commits into from
Sep 26, 2019

Conversation

@estroz
Copy link
Member

@estroz estroz commented Jul 15, 2019

Description of the change: From #1541:

A document describing best practices, tips, gotchas, and any other helpful information about making various API changes in an existing SDK project.

Note: for Go projects only. Ansible and Helm have TODO's.

Motivation for the change: see #1541.

Closes #1541

PTAL @awgreene @dmesser

@estroz estroz added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 15, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2019
@estroz estroz force-pushed the doc-api-changes branch from f3219bb to 80f592e Compare July 15, 2019 21:31
hasbro17
hasbro17 reviewed Jul 22, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Jul 22, 2019
View reviewed changes
This was referenced Jul 22, 2019
Closed
Closed
jmrodri
jmrodri requested changes Jul 26, 2019
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

A few typos.

@estroz estroz requested review from hasbro17 and jmrodri July 29, 2019 15:18
awgreene
awgreene requested changes Jul 29, 2019
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Great work @estroz, a few nits but non-blocking.

doc/user/migrating-existing-apis.md
Kubernetes APIs are assumed to evolve over time, hence the well-defined API [versioning scheme][k8s-versioning]. Upgrading your operator's APIs can be a non-trivial task, one that will involve changing quite a few source files and manifests. This document aims to identify the complexities of migrating an operator project's API using examples from existing operators.

While examples in this guide follow particular types of API migrations, most of the documented migration steps can be generalized to all migration types. A thorough discussion of migration types for a particular project type (Go, Ansible, Helm) is found at the end of each project type's section.

Copy link
Member

@awgreene awgreene Jul 29, 2019

Choose a reason for hiding this comment

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

It may be beneficial to start with a happy case where the GVK contains a single kind.

Copy link
Member Author

@estroz estroz Aug 6, 2019

Choose a reason for hiding this comment

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

You mean do a walkthrough of a migration with a single kind?

Copy link
Member

@awgreene awgreene Aug 7, 2019

Choose a reason for hiding this comment

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

Correct - the first case is a bit more difficult to consume when compared to the case outlined here.

Copy link
Member Author

@estroz estroz Aug 26, 2019

Choose a reason for hiding this comment

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

This case should be added in a follow-up PR since there are a lot of changes here already.

joelanford
joelanford approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM after addressing comments.

jmrodri
jmrodri approved these changes Aug 20, 2019
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 20, 2019
@estroz estroz requested a review from joelanford August 22, 2019 18:17
@estroz
Copy link
Member Author

estroz commented Aug 27, 2019

/test e2e-aws-subcommand
/test e2e-aws-ansible

@hasbro17
Copy link
Contributor

hasbro17 commented Sep 9, 2019

Sorry for the hold up. I'll finish my review on this hopefully in a day or so.

joelanford
joelanford approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2019
hasbro17
hasbro17 reviewed Sep 19, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Sep 19, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Sep 19, 2019
View reviewed changes
hasbro17
hasbro17 approved these changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

Some edits to the CRD manifest naming format but other than that LGTM.

@hasbro17
Apply suggestions from code review
fcca464 
Co-Authored-By: Haseeb Tariq <hasbro17@gmail.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 19, 2019

New changes are detected. LGTM label has been removed.

@estroz
Copy link
Member Author

estroz commented Sep 20, 2019

/test e2e-aws-go
/test e2e-aws-ansible

@estroz
Copy link
Member Author

estroz commented Sep 20, 2019

/test e2e-aws-ansible

1 similar comment
@estroz
Copy link
Member Author

estroz commented Sep 26, 2019

/test e2e-aws-ansible

@estroz estroz changed the title doc/user/migrating-existing-apis.md: document API version migrations doc/user/migrating-existing-apis.md: document API version migra… Sep 26, 2019
@estroz estroz merged commit 0481703 into operator-framework:master Sep 26, 2019
@estroz estroz deleted the doc-api-changes branch September 26, 2019 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@awgreene awgreene awgreene requested changes

@jmrodri jmrodri jmrodri approved these changes

@joelanford joelanford joelanford approved these changes

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

@fabianvf fabianvf Awaiting requested review from fabianvf

@AlexNPavel AlexNPavel Awaiting requested review from AlexNPavel

@lilic lilic Awaiting requested review from lilic

@theishshah theishshah Awaiting requested review from theishshah

+1 more reviewer

@hasbro17 hasbro17 hasbro17 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmrodri jmrodri

@joelanford joelanford

Labels

kind/documentation Categorizes issue or PR as related to documentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Document how to make changes to APIs (e.g. v1 to v2)

6 participants

@estroz @hasbro17 @openshift-ci-robot @jmrodri @joelanford @awgreene