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

use Devfiles for s2i components #4281

Closed
2 of 4 tasks
kadel opened this issue Dec 3, 2020 · 29 comments · Fixed by #4518 or #4683
Closed
2 of 4 tasks

use Devfiles for s2i components #4281

kadel opened this issue Dec 3, 2020 · 29 comments · Fixed by #4518 or #4683
Assignees
Labels
area/refactoring Issues or PRs related to code refactoring estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).

Comments

@kadel
Copy link
Member

kadel commented Dec 3, 2020

Use devfile.yml for s2i components instead of LocalConfig.yaml

Motivation

Odo has two types of components, Devfile and s2i. Currently, each component type has its own separate code path. This means that each command needs to understand how to do the action for Devfile and also for s2i. Technically it is possible to create a Devfile that will mimic what odo is doing with s2i. We already have this in odo utils convert. Odo should leverage that logic and start treating s2i components as regular Devfile. This will reduce the odo code base and will make code maintenance a lot simpler.

odo create --s2i

odo create --s2i command would no longer generate LocalConfig (./odo/config.yml). Instead, it should just generate devfile.yml, and optionally ./odo/env.yml) depending on what will be required.

all other commands

The rest of the commands (like odo storage, odo url, etc..) should not need to know anything about s2i. For them, it will be just another Devfile component.

Acceptance Criteria

  • odo create --s2i should use the s2i to devfile tool to build the devfile on the fly and follow devfile flow in the future.
  • if user has existing LocalConfig s2i component it still needs to work
  • We're going to need to make changes to odo create -h as well.
  • Finally, we're going to need to update docs that end on odo.dev site as well.
@kadel kadel added kind/cleanup priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels Dec 3, 2020
@adisky adisky mentioned this issue Dec 8, 2020
6 tasks
@girishramnani girishramnani self-assigned this Dec 28, 2020
@dharmit
Copy link
Member

dharmit commented Jan 18, 2021

@girishramnani you might want to consider #4201 while working on this. Basically, odo create --s2i with --env should not break if we were to keep compatibility with odo create from v1.

@dharmit
Copy link
Member

dharmit commented Jan 21, 2021

  • Also consider odo link with Service Catalog breaking since this works only between s2i component & SC backed service.
  • Conversion tool (s2i to devfile) itself has issues as observed by @mik-dass. We should fix/address those as a part of this issue.

@dharmit
Copy link
Member

dharmit commented Jan 21, 2021

Need #4369 to go in first, right @girishramnani ?

@girishramnani
Copy link
Contributor

Just so its documented - I dont think --now flag works for devfile so I am not gonna worry about it for now

@girishramnani
Copy link
Contributor

Also this issue will break all s2i components - no existing s2i components would work.

@kadel
Copy link
Member Author

kadel commented Feb 9, 2021

Also this issue will break all s2i components - no existing s2i components would work.

I forgot to mention that in the issue :-(. This can't break existing s2i components without any warning.
We either have to find a way to convert existing s2i components or keep the s2i code there for some time and display a depreciation warning and then after a few releases remove it.

@girishramnani
Copy link
Contributor

we can do a Y stream release to signify such a huge breaking change @kadel

@girishramnani
Copy link
Contributor

We can deprecate s2i components using an early PR that goes in the week’s release and then this PR can be released in later version

@kadel
Copy link
Member Author

kadel commented Feb 10, 2021

we can do a Y stream release to signify such a huge breaking change @kadel

Y stream should not break backward compatibility.

We can deprecate s2i components using an early PR that goes in the week’s release and then this PR can be released in later version

that is too quick.

BTW the original idea for this PR was to just make sure that new components will be all based on Devfile.
But the existing components, even old s2i based on LocalConfig should work as they currently do.

Removal of LocalConfig components would happen later only after we are sure that almost no one is using it.

I have updated Acceptance Criteria to include this

@kadel
Copy link
Member Author

kadel commented Mar 31, 2021

Are we going to have odo create --s2i --port get converted into a devfile with #4518? Or is that scoped for other time?

it should be part of the initial work. Some components won't work without --port support

@girishramnani
Copy link
Contributor

its supported now @dharmit

@girishramnani
Copy link
Contributor

girishramnani commented Apr 7, 2021

Breaking change

Currently a converted s2i component would already have a url defined even if the original s2i component didn't have one, this was done to keep the behavior consistent between s2i and devfile cmps, as a new devfile component creates the url without a user doing "odo url create"

@girishramnani
Copy link
Contributor

girishramnani commented Apr 14, 2021

removing all duplicate tests that are present in devfile and s2i both - Criteria of removal is to exactly match the title of the test and instructions in the tests

Current situation - 10 integration tests failing, 12 e2e tests failing.
Fixed 5 today, more underway.

@girishramnani
Copy link
Contributor

Currently I cannot debug e2e tests because they are timing out on prow.

@girishramnani
Copy link
Contributor

Fixed all the tests but gingko is timing out, I tried to reproduce this timeout but it didn’t happen so thinking about other ways to fix this

@girishramnani
Copy link
Contributor

Some questions asked so far
#4518 (comment)

@serenamarie125 serenamarie125 added this to the 2.2 (Planning) milestone Apr 20, 2021
@serenamarie125 serenamarie125 added estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person and removed estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person labels Apr 20, 2021
@girishramnani girishramnani reopened this Apr 20, 2021
@girishramnani
Copy link
Contributor

Need to add help and update docs

@girishramnani
Copy link
Contributor

But I went through the odo.dev docs, there are 20 pages of doc and nearly no mention of —s2i.

We need 2-3 paragraphs explaining —git, —binary and other 2 issues.

And the demo video needs to change.
Will share more of my findings

@girishramnani
Copy link
Contributor

Current plan -
I am writing a page in odo.dev titled “breaking changes in odo 2.2”
Explaining my —s2i change. I wasn’t sure where to document this so this seemed like the best option

@girishramnani
Copy link
Contributor

Docs #4683

@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023
@rm3l rm3l added this to odo Project Jun 16, 2023
@rm3l rm3l moved this to Done ✅ in odo Project Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/refactoring Issues or PRs related to code refactoring estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects
Archived in project
5 participants