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

docs: tekton and crane ui integration #59

Merged
merged 5 commits into from Feb 17, 2022

Conversation

djzager
Copy link
Member

@djzager djzager commented Jan 28, 2022

The Tekton + Crane UI Integration enhancement.

@djzager djzager marked this pull request as ready for review January 31, 2022 18:26
@djzager
Copy link
Member Author

djzager commented Jan 31, 2022

cc @jmontleon @JaydipGabani

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

I think that we should consider some approach for versioning on the cluster tasks.

I also believe that we should use the user's oath in some way to have access to the destination cluster rather an trying to rely on an admin user always being present.

@mturley
Copy link

mturley commented Feb 1, 2022

Just to copy here: there are a few properties the UI collects (as designed currently) that I don't see how to map to params on the ClusterTasks here for each PVC: migration type (from prior discussion it sounds like this may not be relevant), target storage class name, capacity, and a boolean for whether to verify the copy.

Slack discussion: https://coreos.slack.com/archives/C02ELCA262K/p1643662825288189

Copy link
Member

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

Overall I think this is a really thorough enhancement and it satisfies our need to get our design thoughts captured so we can begin the implementation. There are several open questions we should document and in some cases defer to follow-on enhancements. Let's just make sure those are all called out, and I will make sure that we have spikes on the roadmap to get those questions answered. Thank you for this @djzager, I learned a lot with the read.

enhancements/crane-2.0/tekton-ui-integration/README.md Outdated Show resolved Hide resolved
enhancements/crane-2.0/tekton-ui-integration/README.md Outdated Show resolved Hide resolved

#### crane-kubectl-scale-down

The `kubectl-scale-down` ClusterTask is responsible for scaling down the
Copy link
Member

Choose a reason for hiding this comment

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

@pranavgaikwad is this task sufficient to accomplish the abstract idea of "Quiesce"? What does MTC do if they are one-off pods with volumes mounted?

enhancements/crane-2.0/tekton-ui-integration/README.md Outdated Show resolved Hide resolved
enhancements/crane-2.0/tekton-ui-integration/README.md Outdated Show resolved Hide resolved
greatest probably of API incompatibilities are from adding/removing parameters
or workspaces from ClusterTasks.

To help mitigate these risks, whatever container image specified for Crane
Copy link
Member

Choose a reason for hiding this comment

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

I think an assertion by the pipeline that these are all fall within acceptable and expected versions and it fails fast and loudly with a specific error would be a good thing to add.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you expand on this?

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Contributor

@alaypatel07 alaypatel07 left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough write up, overall it makes a lot of sense.

/lgtm

djzager added a commit to djzager/crane-runner that referenced this pull request Feb 16, 2022
The purpose of this change is to align with what is described in
konveyor/enhancements#59 only to the extent
that a demonstration of "Stateless App Migration" is possible.

This will likely break our examples so it should not be merged into
'main' until stabilized.
@eriknelson eriknelson merged commit 45310ad into konveyor:master Feb 17, 2022
djzager added a commit to djzager/crane-runner that referenced this pull request Feb 22, 2022
The purpose of this change is to align with what is described in
konveyor/enhancements#59 only to the extent
that a demonstration of "Stateless App Migration" is possible.

This will likely break our examples so it should not be merged into
'main' until stabilized.
djzager added a commit to migtools/crane-runner that referenced this pull request Mar 18, 2022
* feat: update clustertasks to integrate with UI

The purpose of this change is to align with what is described in
konveyor/enhancements#59 only to the extent
that a demonstration of "Stateless App Migration" is possible.

This will likely break our examples so it should not be merged into
'main' until stabilized.

* chore: move kustomize manifests to config

* chore: update examples based on new paths

Now that all of the ClusterTasks have been updated based on
[the tekton ui integration enhancement](https://github.com/konveyor/enhancements/tree/master/enhancements/crane-2.0/tekton-ui-integration),
and some issues were discovered with handling of defaults, this commit
attempts to make all of the changes functional. That means, after this
commit all examples should integrate with the ClusterTasks as
implemented.

* chore: update hack dir

* chore: remove personal container image references

* docs: update descriptions to be more meaningful
pranavgaikwad pushed a commit to pranavgaikwad/crane-runner that referenced this pull request Mar 29, 2022
The purpose of this change is to align with what is described in
konveyor/enhancements#59 only to the extent
that a demonstration of "Stateless App Migration" is possible.

This will likely break our examples so it should not be merged into
'main' until stabilized.
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.

None yet

5 participants