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

Refactor underlying code for "odo service" commands #4242

Closed
6 tasks done
dharmit opened this issue Nov 19, 2020 · 10 comments
Closed
6 tasks done

Refactor underlying code for "odo service" commands #4242

dharmit opened this issue Nov 19, 2020 · 10 comments
Labels
area/refactoring Issues or PRs related to code refactoring lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). triage/unresolved Indicates an issue that can not or will not be resolved. v2 Issue or PR that applies to the v2 of odo

Comments

@dharmit
Copy link
Member

dharmit commented Nov 19, 2020

Currently odo service supports two service backends:

  1. Service Catalog
  2. Operator Hub

Because of the numerous if..else blocks and short term thinking we did on this during v2 release, we've accumulated code that's:

  1. not easy to read
  2. difficult to debug
  3. buggy (list of bugs)
  4. difficult to add features without adding more if..else blocks

Besides that, we might plan to add support for other service backends future. Thus, making the code easy to add more pending features w.r.t Operator Hub integration (#2613), and easy to plug that extra backend (if we decide to) is of utmost importance.

Acceptance Criteria / Related Issues / Dependent Issues

/kind code-refactoring
/area service
/area service-operators

@dharmit
Copy link
Member Author

dharmit commented Jan 20, 2021

Scope of work for refactoring odo service create:

  1. We will decide between using Operator Hub or Service Catalog backend by looking at the arguments passed.
    # below command will assume Operator Hub backend because of a forward slash
    $ odo service create etcdoperator.v0.9.4/EtcdCluster
    
    # below command will assume Service Catalog backend because there's no forward slash
    $ odo service create dh-postgresql-apb --plan dev -p postgresql_user=luke -p postgresql_password=secret -p postgresql_database=my_data -p postgresql_version=9.6 -w
  2. We need an interface that would have methods to create a service.
  3. I will be modifying the struct ServiceCreateOptions to use this interface. https://github.com/openshift/odo/blob/5544b25ec083506393090faad5b4205126e4f675/pkg/odo/cli/service/create.go#L61-L62 Fields specific to Service Catalog and Operator Hub will be moved to their respective structs and these types will implement the interface we create for odo service create. In its simplest form, all the code from Comlpete, Validate and Run functions will be moved to methods of the same name implemented by the two structs.
  4. As for the integration tests, the ones written currently should pass without any major modification.
  5. As for the unit tests, more of those will be added in this refactor.

Other odo service commands to follow the suite. Separate interfaces will be created for them. I will try to keep smaller interface with lesser/specific methods so that we can plug in odo link support for Helm without much issues.

For odo link, I will put off refactoring till #4281 is addressed because with that going in, we will be having to deal with only devfile components. More importantly, I think it will be done long before I reach the point of refactoring odo link.

@faysou
Copy link

faysou commented Jan 21, 2021

@dharmit I'm looking forward to be able to add a postgres db with odo for local development in CRC. It seems no service is available when using CRC, except ones added through an operator, but this makes things harder, I've been spending a few hours on adding one of the postgres operators available and making it work in odo but still didn't manage to.

It would be easier to have access to the Catalog templates from a user perspective (from a dev perspective I see you have been following this issue and the solution seems not obvious).

Alternatively is there some tutorial somewhere on how to develop locally without odo ?
Following this tutorial, I could make it work with oc but not with odo.
https://github.com/redhat-developer/s2i-dotnetcore-persistent-ex
Edit: I actually just managed to make odo work in the context of the tutorial above by combining the odo way and adding a database and secrets using oc instead of odo, so I mixed the odo and oc way, this still allows me to get benefits of local development with odo.

Thanks a lot for your work.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 21, 2021
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 21, 2021
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci
Copy link

openshift-ci bot commented Jun 20, 2021

@openshift-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

@openshift-ci openshift-ci bot closed this as completed Jun 20, 2021
@prietyc123
Copy link
Contributor

Acceptance criteria has not been met.

/reopen
/lifecycle frozen

@openshift-ci
Copy link

openshift-ci bot commented Aug 11, 2021

@prietyc123: Reopened this issue.

In response to this:

Acceptance criteria has not been met.

/reopen
/lifecycle frozen

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.

@openshift-ci openshift-ci bot reopened this Aug 11, 2021
@openshift-ci openshift-ci bot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Aug 11, 2021
@dharmit
Copy link
Member Author

dharmit commented Aug 17, 2021

Acceptance criteria has not been met.

/reopen
/lifecycle frozen

@prietyc123 This issue was opened as a part of "refactoring" the odo code right after v2 was released. But the time we had taken to "refactor" things didn't go well.

After that, it was decided that we would refactor the code when opening PRs for features/bugs, but won't dedicatedly work on refactoring things. If you look at #4465, it refactors a lot of code of odo service commands so that we use Go interfaces instead of if..else mess we were stuck with.

I'm removing the lifecycle frozen from this issue, but I'd urge you to close it, as we're not going to work dedicatedly on refactoring things.

/remove-lifecycle frozen
/triage unresolved

@openshift-ci openshift-ci bot added triage/unresolved Indicates an issue that can not or will not be resolved. and removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Aug 17, 2021
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2021
@dharmit dharmit closed this as completed Feb 2, 2022
@dharmit dharmit removed their assignment Feb 2, 2022
@rm3l rm3l added the v2 Issue or PR that applies to the v2 of odo label Jun 16, 2023
@rm3l rm3l added this to odo Project Jun 16, 2023
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
@rm3l rm3l moved this to Done ✅ in odo Project Jun 23, 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 lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). triage/unresolved Indicates an issue that can not or will not be resolved. v2 Issue or PR that applies to the v2 of odo
Projects
Archived in project
Development

No branches or pull requests

6 participants