Convert s2i components to devfile #3549

adisky · 2020-07-13T09:59:23Z

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
Convert s2i local components to devfile.yaml

Which issue(s) this PR fixes:

fixes #3156

How to test changes / Special notes to the reviewer:

Deploy a s2i based component using odo
$ export ODO_BOOTSTRAPPER_IMAGE=adisky/odo-init-image:8.0
(due to these changes Added ODO_S2I_CONVERTED_DEVFILE env variable odo-init-image#70)
$ odo preference set experimental false
$ odo create nodejs test-nodejs
$ odo url create
$ odo push

Now Convert this s2i component to devfile

$ odo preference set experimental true
$ odo utils convert-to-devfile (creates devfile.yaml, env.yaml)
$ odo push (deploys devfile component)
$ odo delete --s2i (deletes s2i component)

girishramnani · 2020-07-14T08:39:43Z

pkg/devfile/parser/data/2.0.0/components.go

+
+// AddComponent adds component to devfile
+func (d *Devfile200) AddComponent(component common.DevfileComponent) {
+	d.Components = append(d.Components, component)


should we check for duplicates?

yes, updating

dharmit · 2020-07-15T11:42:11Z

Few observations/questions:

odo utils migrate-to-devfile command works without Experimental mode as well. But, and obviously, it doesn't push using the Devfile.

While not incorrect, the YAML generated by the command has metadata and schema info at the bottom unlike the top where we're used to seeing it. Should we make sure that it shows up at the top?

commands:
- exec:
    commandLine: /opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart
    component: s2i-builder
    group:
      isDefault: true
      kind: build
    id: s2i-assemble
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: run
    id: s2i-run
components:
- container:
    endpoints:
    - configuration:
        public: true
      name: port-8080
      targetPort: 8080
    env:
    - name: ODO_S2I_SCRIPTS_URL
      value: /usr/libexec/s2i
    - name: ODO_S2I_SCRIPTS_PROTOCOL
      value: image://
    - name: ODO_S2I_SRC_BIN_PATH
      value: /tmp
    - name: ODO_S2I_DEPLOYMENT_DIR
      value: /opt/app-root/src
    - name: ODO_S2I_WORKING_DIR
      value: /opt/app-root/src
    - name: ODO_S2I_BUILDER_IMG
      value: rhscl/nodejs-12-rhel7
    - name: ODO_SRC_BACKUP_DIR
      value: /opt/app-root/src-backup
    - name: ODO_S2I_CONVERTED_DEVFILE
      value: "true"
    image: registry.redhat.io/rhscl/nodejs-10-rhel7:latest
    mountSources: true
    name: s2i-builder
    sourceMapping: /tmp/projects
metadata:
  name: nodejs-nodejs-ex-qaxb
  version: 1.0.0
schemaVersion: 2.0.0

Doing odo push doesn't delete the old pod, route and service from the cluster. (there could be more dangling objects but these are the ones I observed.) Should we delete them as a part of this PR or a different PR?

adisky · 2020-07-15T12:03:04Z

Few observations/questions:

odo utils migrate-to-devfile command works without Experimental mode as well. But, and obviously, it doesn't push using the Devfile.

I think it is not a problem, anyways we are taking out devfile from experimental mode

While not incorrect, the YAML generated by the command has metadata and schema info at the bottom unlike the top where we're used to seeing it. Should we make sure that it shows up at the top?

commands:
- exec:
    commandLine: /opt/odo/bin/s2i-setup && /opt/odo/bin/assemble-and-restart
    component: s2i-builder
    group:
      isDefault: true
      kind: build
    id: s2i-assemble
- exec:
    commandLine: /opt/odo/bin/run
    component: s2i-builder
    group:
      isDefault: true
      kind: run
    id: s2i-run
components:
- container:
    endpoints:
    - configuration:
        public: true
      name: port-8080
      targetPort: 8080
    env:
    - name: ODO_S2I_SCRIPTS_URL
      value: /usr/libexec/s2i
    - name: ODO_S2I_SCRIPTS_PROTOCOL
      value: image://
    - name: ODO_S2I_SRC_BIN_PATH
      value: /tmp
    - name: ODO_S2I_DEPLOYMENT_DIR
      value: /opt/app-root/src
    - name: ODO_S2I_WORKING_DIR
      value: /opt/app-root/src
    - name: ODO_S2I_BUILDER_IMG
      value: rhscl/nodejs-12-rhel7
    - name: ODO_SRC_BACKUP_DIR
      value: /opt/app-root/src-backup
    - name: ODO_S2I_CONVERTED_DEVFILE
      value: "true"
    image: registry.redhat.io/rhscl/nodejs-10-rhel7:latest
    mountSources: true
    name: s2i-builder
    sourceMapping: /tmp/projects
metadata:
  name: nodejs-nodejs-ex-qaxb
  version: 1.0.0
schemaVersion: 2.0.0

I don't know how to preserve order for a generated yaml, would check about this.

Doing odo push doesn't delete the old pod, route and service from the cluster. (there could be more dangling objects but these are the ones I observed.) Should we delete them as a part of this PR or a different PR?

isn't it expected? I believe that if the new devfile component deployed successfully user then deletes the old one?
and odo push is not automatically doing this as with devfiles we use deployment and with s2i we use deployment config. So there is no update here it is creating a new component.

even if we want to delete it, it should ne be done with odo push. If we want that, odo utils migrate-to-devfile push the new one and deletes the old one.

adisky · 2020-07-15T14:41:37Z

@dharmit addressed your comments for ordering the yaml, now it is generating in order.

mik-dass · 2020-07-21T05:06:25Z

pkg/odo/cli/utils/migrate.go

+		return errors.Wrap(err, "Error in generating env.yaml")
+	}
+
+	// TODO: Delete the s2i component and deploy the devfile component.


It is just a note about the data we are unable to migrate.

Please update with some reasoning and also the link to the issue if available.

mik-dass · 2020-07-21T05:06:45Z

pkg/odo/cli/utils/migrate.go

+	/*  This data is yet to be converted
+
+	// Absolute path
+	sourcePath, _ := context.LocalConfigInfo.GetOSSourcePath()
+	minMemory := context.LocalConfigInfo.GetMinMemory()
+	minCPU := context.LocalConfigInfo.GetMinCPU()
+	maxCPU := context.LocalConfigInfo.GetMaxCPU()
+
+	*/


seems like a TODO

mik-dass · 2020-07-21T05:06:56Z

pkg/odo/cli/utils/migrate.go

+// TODO
+// lables and annotations that are added on s2i components


mik-dass · 2020-07-21T05:08:45Z

pkg/odo/cli/utils/migrate.go

+	if err != nil {
+		return err
+	}
+	log.Italic("devfile.yaml is available in current directory, run `odo push` to deploy devfile component and `odo delete` to delete s2i component.\n")


won't running odo delete after push delete the devfile component?

you have an s2i component in folder

cd my-s2i-component

run odo utils migrate-to-devfile (generates devfile.yaml, env.yaml)

odo push (since devfile has priority over s2i, devfile component would get deployed. s2i would is still exisiting)

odo delete(s2i has priority over for delete, so s2i would get deleted)

you have now running devfile component.

This sounds confusing TBH. How about we suggest delete before push? Will that work?

yes, but what would happen when odo push for devfile gets failed and user lost the old s2i component as well?
in ideal migration strategy the when the new deployment gets successful then only the old one rolled back.

OK 👍 Please add a comment regarding this.

mik-dass · 2020-07-21T05:16:16Z

pkg/odo/cli/utils/migrate.go

+// NewCmdMigrate implements the odo utils migrate-to-devfile command
+func NewCmdMigrate(name, fullName string) *cobra.Command {
+	o := NewMigrateOptions()
+	migrateCmd := &cobra.Command{


please add some integration tests for this command.

would update

adisky · 2020-07-21T07:10:22Z

@mik-dass what is the problem with TODO's in code? these are for future references like app label is still under development.

mik-dass · 2020-07-21T07:18:43Z

@mik-dass what is the problem with TODO's in code? these are for future references like app label is still under development.

Some of them are confusing as to why they are not a part of this PR. We should provide proper reasoning as to why they are not implemented in this PR. Links to known issues are even better. Without them, the PR review becomes confusing.

adisky · 2020-07-21T07:27:49Z

Some of them are confusing as to why they are not a part of this PR. We should provide proper reasoning as to why they are not implemented in this PR. Links to known issues are even better. According to me, it makes the PR review confusing.

yes correct, so you should give comment like please elaborate TODO, only TODO found would not make the author understand what reviewer wants to say.

mik-dass · 2020-07-21T07:34:58Z

yes correct, so you should give comment like please elaborate TODO, only TODO found would not make the author understand what reviewer wants to say.

That's how I have always started a conversation regarding the TODO as the details has not been provided and I end up assuming it to be personal notes for the future. He/she can always reply back in the review comment. Anyways, will keep this in mind next time.

adisky · 2020-07-21T10:45:36Z

as discussed with @kadel and feedback from @mik-dass, delete operation looks confusing here, we have decided to introduce a flag --s2i for odo delete. Marking it as WIP till i update the PR.

adisky · 2020-07-28T08:14:08Z

/retest

tests/integration/component.go

kadel · 2020-07-29T10:35:29Z

/retest

mik-dass · 2020-07-29T11:49:55Z

pkg/odo/cli/utils/convert.go

+		endpoint := common.Endpoint{
+			Name:       fmt.Sprintf("port-%s", port[0]),
+			TargetPort: int32(portInt),
+			Configuration: &common.Configuration{


I think configuration was removed from the schema recently https://github.com/devfile/kubernetes-api/blob/master/schemas/devfile.json#L439

check if s2i component is present for deletion first

update 2.0 go struct to preserve yaml order

rename it to convert add --s2i flag for odo delete

This reverts commit 06392de.

This reverts commit 18c142e.

openshift-ci-robot · 2020-07-29T15:50:40Z

@adisky: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/v4.2-integration-e2e	`f4eb04f`	link	`/test v4.2-integration-e2e`
ci/prow/v4.3-integration-e2e	`f4eb04f`	link	`/test v4.3-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.

adisky · 2020-07-30T04:05:15Z

/retest

mik-dass

Works for me
/lgtm

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Jul 13, 2020

openshift-ci-robot requested review from cdrage and prietyc123 July 13, 2020 09:59

adisky force-pushed the s2idevfile branch 2 times, most recently from 78d84d3 to 4ea1743 Compare July 13, 2020 11:53

girishramnani reviewed Jul 14, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 14, 2020

adisky force-pushed the s2idevfile branch from 1f33f1d to 3c17929 Compare July 14, 2020 09:46

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 14, 2020

adisky force-pushed the s2idevfile branch from 3c17929 to f7c51a7 Compare July 14, 2020 12:50

adisky force-pushed the s2idevfile branch from f7c51a7 to 69269ff Compare July 15, 2020 12:53

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 18, 2020

adisky force-pushed the s2idevfile branch from 536985c to 9e357cd Compare July 20, 2020 12:24

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 20, 2020

adisky force-pushed the s2idevfile branch from 9e357cd to 6d8f416 Compare July 20, 2020 12:52

mik-dass reviewed Jul 21, 2020

View reviewed changes

adisky changed the title ~~Convert s2i components to devfile~~ [WIP] Convert s2i components to devfile Jul 21, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 21, 2020

mik-dass reviewed Jul 28, 2020

View reviewed changes

tests/integration/component.go Show resolved Hide resolved

mik-dass reviewed Jul 29, 2020

View reviewed changes

Aditi Sharma added 18 commits July 29, 2020 17:23

odo migrate to devfile

1e59e66

Update yaml tags.

9c8568d

Implemented s2i to devfile conversion for local components

ccc265e

Fix unit tests

748f874

Add error message for git components

2ae4f19

Refactor odo delete logic

bb6d2df

check if s2i component is present for deletion first

Revert odo delete change

edea12c

Preserve yaml order

fa3dc1a

update 2.0 go struct to preserve yaml order

Add Check for duplicates

0b88d4a

Remove go.sum changes

989dbc1

Update the output and help message

e9b0845

rename it to convert add --s2i flag for odo delete

convert app label and add integration test

70d6bf8

Disable the integration test

ee660d6

Rebase commit

4a416fd

Address review comments

2e752d2

Rebase and address review comments

87d596f

Revert "Remove configuration from devfiles"

18c142e

This reverts commit 06392de.

Rebase and update as per new schema

e982ec1

adisky force-pushed the s2idevfile branch from 058a0ea to e982ec1 Compare July 29, 2020 12:13

Revert "Revert "Remove configuration from devfiles""

2c9a67b

This reverts commit 18c142e.

mik-dass approved these changes Jul 30, 2020

View reviewed changes

openshift-ci-robot assigned mik-dass Jul 30, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 30, 2020

openshift-merge-robot merged commit 8b87f67 into redhat-developer:master Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert s2i components to devfile #3549

Convert s2i components to devfile #3549

adisky commented Jul 13, 2020 •

edited

Loading

girishramnani Jul 14, 2020

adisky Jul 14, 2020

adisky Jul 20, 2020

dharmit commented Jul 15, 2020

adisky commented Jul 15, 2020 •

edited

Loading

adisky commented Jul 15, 2020

mik-dass Jul 21, 2020

adisky Jul 21, 2020

mik-dass Jul 21, 2020 •

edited

Loading

mik-dass Jul 21, 2020

mik-dass Jul 21, 2020

mik-dass Jul 21, 2020 •

edited

Loading

adisky Jul 21, 2020

mik-dass Jul 21, 2020

adisky Jul 21, 2020

mik-dass Jul 21, 2020

mik-dass Jul 21, 2020

adisky Jul 21, 2020

adisky commented Jul 21, 2020

mik-dass commented Jul 21, 2020 •

edited

Loading

adisky commented Jul 21, 2020

mik-dass commented Jul 21, 2020 •

edited

Loading

adisky commented Jul 21, 2020

adisky commented Jul 28, 2020

kadel commented Jul 29, 2020

mik-dass Jul 29, 2020

openshift-ci-robot commented Jul 29, 2020 •

edited

Loading

adisky commented Jul 30, 2020

mik-dass left a comment

		// TODO
		// lables and annotations that are added on s2i components

Convert s2i components to devfile #3549

Convert s2i components to devfile #3549

Conversation

adisky commented Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharmit commented Jul 15, 2020

adisky commented Jul 15, 2020 • edited Loading

adisky commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisky commented Jul 21, 2020

mik-dass commented Jul 21, 2020 • edited Loading

adisky commented Jul 21, 2020

mik-dass commented Jul 21, 2020 • edited Loading

adisky commented Jul 21, 2020

adisky commented Jul 28, 2020

kadel commented Jul 29, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 29, 2020 • edited Loading

adisky commented Jul 30, 2020

mik-dass left a comment

Choose a reason for hiding this comment

adisky commented Jul 13, 2020 •

edited

Loading

adisky commented Jul 15, 2020 •

edited

Loading

mik-dass Jul 21, 2020 •

edited

Loading

mik-dass Jul 21, 2020 •

edited

Loading

mik-dass commented Jul 21, 2020 •

edited

Loading

mik-dass commented Jul 21, 2020 •

edited

Loading

openshift-ci-robot commented Jul 29, 2020 •

edited

Loading