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

Guide for migrating from v2 to v3 #6095

Merged
merged 5 commits into from
Sep 16, 2022
Merged

Guide for migrating from v2 to v3 #6095

merged 5 commits into from
Sep 16, 2022

Conversation

dharmit
Copy link
Member

@dharmit dharmit commented Sep 5, 2022

Signed-off-by: Dharmit Shah shahdharmit@gmail.com

What type of PR is this:
/kind documentation

What does this PR do / why we need it:
$subject

Which issue(s) this PR fixes:
Fixes #6069

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@netlify
Copy link

netlify bot commented Sep 5, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 2f1bc09
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6322d3a1b318040009294a4e
😎 Deploy Preview https://deploy-preview-6095--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@odo-robot
Copy link

odo-robot bot commented Sep 5, 2022

Unit Tests on commit 8fdb2b8 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 5, 2022

Validate Tests on commit 8fdb2b8 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 5, 2022

Kubernetes Tests on commit 8fdb2b8 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 5, 2022

OpenShift Tests on commit 8fdb2b8 finished successfully.
View logs: TXT HTML

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Added a few comments.

I think it is worth documenting the following changes (with more concrete examples):

  • how we leverage port-forwarding instead of URLs to access the running application. A concrete example of how to access the application using both v2 and v3 could be helpful, I think.
  • what needs to be changed for debugging to work with v3: if I'm not mistaken, if v2, you needed to run odo push --debug and then odo debug port-forward. In v3, a Debug endpoint needs to be explicitly declared in the Devfile.

docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
@dharmit
Copy link
Member Author

dharmit commented Sep 6, 2022

  • how we leverage port-forwarding instead of URLs to access the running application. A concrete example of how to access the application using both v2 and v3 could be helpful, I think.

I agree that this needs to be mentioned, but don't think it should be done with an example for v2 and v3. I have added a section titled "What happened to Ingress/Route?" to clarify this.

  • what needs to be changed for debugging to work with v3: if I'm not mistaken, if v2, you needed to run odo push --debug and then odo debug port-forward. In v3, a Debug endpoint needs to be explicitly declared in the Devfile.

I have similarly added a section titled "Changes to the way component debugging works" for this.

@odo-robot
Copy link

odo-robot bot commented Sep 6, 2022

Windows Tests (OCP) on commit 8fdb2b8 finished with errors.
View logs: TXT HTML

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Added more comments.

LGTM overall, but while reviewing and testing the migration, I was also thinking about the following "breaking" changes that might be worth documenting here:

  • how odo dev handles command and args in the container definition in the Devfile (Add support for command/args fields in container components #5768). I am not sure how v2 users used this, but this is a potential breaking change, I think: in v2, the container command was always overridden with Supervisord; but v3 now respects what has been specified in the Devfile as command/args. However, v3 requires a non-terminating command in the Devfile, so the container can remain running. If nothing is defined, odo dev defaults to tail -f /dev/null.

  • the fact that Ephemeral now defaults to False (Change ephemeral default to false #5795), and what this means for users

  • the changes in the odo config file related to temporal values (#5822). This point might not be that critical to document here, because the odo commands will display warnings like ⚠ Please change the preference value for Timeout, PushTimeout, RegistryCacheTime, the value does not comply with the minimum value of 1s; e.g. of acceptable formats: 4s, 5m, 1h. But I was wondering if we shouldn't mention it here too.

docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 8, 2022
@dharmit
Copy link
Member Author

dharmit commented Sep 9, 2022

how odo dev handles command and args in the container definition in the Devfile (#5768). I am not sure how v2 users used this, but this is a potential breaking change, I think: in v2, the container command was always overridden with Supervisord; but v3 now respects what has been specified in the Devfile as command/args. However, v3 requires a non-terminating command in the Devfile, so the container can remain running. If nothing is defined, odo dev defaults to tail -f /dev/null.

I think this doesn't affect the users in terms of migration. In v2, we used supervisord. In v3, we override their command with the tail command. However, I do agree that this is something that would be very valuable for odo dev documentation.

the fact that Ephemeral now defaults to False (#5795), and what this means for users

Ack. I'll add.

the changes in the odo config file related to temporal values (#5822). This point might not be that critical to document here, because the odo commands will display warnings like ⚠ Please change the preference value for Timeout, PushTimeout, RegistryCacheTime, the value does not comply with the minimum value of 1s; e.g. of acceptable formats: 4s, 5m, 1h. But I was wondering if we shouldn't mention it here too.

I'd prefer not to mention it here because, I think, it's not going to affect a lot of users (telemetry data could confirm, I'm simply assuming based on issues we have seen around odo preference set of commands). And even to the users it will affect, odo shows a warning message as you pointed.

Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 9, 2022
@rm3l
Copy link
Member

rm3l commented Sep 9, 2022

how odo dev handles command and args in the container definition in the Devfile (#5768). I am not sure how v2 users used this, but this is a potential breaking change, I think: in v2, the container command was always overridden with Supervisord; but v3 now respects what has been specified in the Devfile as command/args. However, v3 requires a non-terminating command in the Devfile, so the container can remain running. If nothing is defined, odo dev defaults to tail -f /dev/null.

I think this doesn't affect the users in terms of migration. In v2, we used supervisord. In v3, we override their command with the tail command. However, I do agree that this is something that would be very valuable for odo dev documentation.

Just to clarify: in v3, we don't override the command but use whatever is defined as command/args in the Devfile container component. As a workaround to have a running container, we force the use of tail only if there is no command/args defined for this Devfile container component. But indeed, this is already somewhat mentioned in odo dev documentation: https://odo.dev/docs/command-reference/dev/#components

My concern was more about people using v2 and who currently have command/args explicitly defined in their Devfiles. Just wanted this doc to warn them to make sure that such definition should result in a non-terminating container.
But you're right, this should be in odo dev documentation instead.

the fact that Ephemeral now defaults to False (#5795), and what this means for users

Ack. I'll add.

Thanks.

the changes in the odo config file related to temporal values (#5822). This point might not be that critical to document here, because the odo commands will display warnings like ⚠ Please change the preference value for Timeout, PushTimeout, RegistryCacheTime, the value does not comply with the minimum value of 1s; e.g. of acceptable formats: 4s, 5m, 1h. But I was wondering if we shouldn't mention it here too.

I'd prefer not to mention it here because, I think, it's not going to affect a lot of users (telemetry data could confirm, I'm simply assuming based on issues we have seen around odo preference set of commands). And even to the users it will affect, odo shows a warning message as you pointed.

Makes sense.

Thanks Dharmit!

@cdrage
Copy link
Member

cdrage commented Sep 13, 2022

This should be under "Advanced Usage" as it messed up the side bar order a bit (or if not in advanced usage, can be at the very end instead / low priority?)

docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
---

### Migrate existing odo component from v2 to v3
If you have created an odo component using odo v2, this section will help you move it to use odo v3.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you have created an odo component using odo v2, this section will help you move it to use odo v3.
If you have created an odo component using odo v2, this section will help you move it to use odo v3.

Copy link
Member

Choose a reason for hiding this comment

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

add newline :)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this may be better as you are just rewording the title?

Suggested change
If you have created an odo component using odo v2, this section will help you move it to use odo v3.
Before migrating to odo v3 you must delete your application and re-run it:

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 don't understand what exactly you are suggesting here. Replacing the current statement with Before migrating to odo v3 you must delete your application and re-run it: is not making sense to me.


### Migrate existing odo component from v2 to v3
If you have created an odo component using odo v2, this section will help you move it to use odo v3.
1. `cd` into the component directory, and delete the component from the Kubernetes cluster:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. `cd` into the component directory, and delete the component from the Kubernetes cluster:
Step 1. `cd` into the component directory, and delete the component from the Kubernetes cluster:

Copy link
Member

Choose a reason for hiding this comment

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

Step 1, step 2, etc.

Copy link
Member

Choose a reason for hiding this comment

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

You also don't necessarily need to cd into the directory ;)

Perhaps just say: "Delete the component from the Kubernetes cluster:"

Copy link
Member Author

Choose a reason for hiding this comment

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

You also don't necessarily need to cd into the directory ;)

I'd prefer having it because if the user wants to odo dev the same component using v3, they need to be in the component directory. So although it's technically possible to delete the component from outside the component directory, I'd rather they cd into it. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Step 1, step 2, etc.

Done. But PTAL and suggest. Please mark resolved if looks good.

docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
docs/website/docs/user-guides/v3-migration-guide.md Outdated Show resolved Hide resolved
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
@dharmit
Copy link
Member Author

dharmit commented Sep 14, 2022

@cdrage thanks for the exhaustive review! I have not committed your suggestions through the GH interface because that would trigger a Netlify deploy preview for each such commit. I have manually made the changes or made a counterpoint. Hopefully, I didn't miss anything in your review.

@rm3l Added the info about Ephemeral storage. PTAL and thanks for your patience. :)

Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@rm3l rm3l 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 changes!

/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l

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

The pull request process is described 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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 15, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 15, 2022

@dharmit: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v4.11-integration-e2e cc701fa link true /test v4.11-integration-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rm3l
Copy link
Member

rm3l commented Sep 15, 2022

Looking into why the test below was not passing on Windows, but this is unrelated to the changes in this PR.

/override windows-integration-test/Windows-test

+ [FAILED] [248.102 seconds]
odo dev command tests
C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:31
  port-forwarding for the component
  C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:471
    when devfile has multiple endpoints
    C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:548
      when running odo dev
      C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:555
        [It] should expose all endpoints on localhost regardless of exposure
        C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:573

    Replacing "Hello from Node.js" with "H3110 from Node.js" in server.js
    [odo] I0915 02:46:34.401942    7144 service.go:122] Getting instances of: servicebindings.servicebinding.io
    [odo]  X  Dev mode interrupted by user
    [odo] Cleaning resources, please wait

Timed out after 180.019s.
  Expected
      <string>: 
  To satisfy at least one of these matchers: [%!s(*matchers.ContainSubstringMatcher=&{Pushing files... []}) %!s(*matchers.ContainSubstringMatcher=&{Updating Component... []})]

@openshift-ci
Copy link

openshift-ci bot commented Sep 15, 2022

@rm3l: Overrode contexts on behalf of rm3l: windows-integration-test/Windows-test

In response to this:

Looking into why the test below was not passing on Windows, but this is unrelated to the changes in this PR.

/override windows-integration-test/Windows-test

+ [FAILED] [248.102 seconds]
odo dev command tests
C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:31
 port-forwarding for the component
 C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:471
   when devfile has multiple endpoints
   C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:548
     when running odo dev
     C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:555
       [It] should expose all endpoints on localhost regardless of exposure
       C:/Users/Administrator.ANSIBLE-TEST-VS/2002/tests/integration/cmd_dev_test.go:573

   Replacing "Hello from Node.js" with "H3110 from Node.js" in server.js
   [odo] I0915 02:46:34.401942    7144 service.go:122] Getting instances of: servicebindings.servicebinding.io
   [odo]  X  Dev mode interrupted by user
   [odo] Cleaning resources, please wait

Timed out after 180.019s.
 Expected
     <string>: 
 To satisfy at least one of these matchers: [%!s(*matchers.ContainSubstringMatcher=&{Pushing files... []}) %!s(*matchers.ContainSubstringMatcher=&{Updating Component... []})]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cdrage
Copy link
Member

cdrage commented Sep 16, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 16, 2022
@openshift-merge-robot openshift-merge-robot merged commit b87eaf7 into redhat-developer:main Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

document differences and migration from v2 to v3
4 participants