Refactoring Context

[adisky]

Some ideas to refactor context

• Only make available single context function outside package, as with multiple functions it behaves differently everywhere https://github.com/openshift/odo/blob/master/pkg/odo/genericclioptions/context.go#L31 add parameters to specify devfile/s2i, or if possible move the devfile/s2i detection to context, internally detect if clusterAccess is required for the command or not.

• Avoid using util.LogErrorAndExit(err, "") at every where in the code. All other functions to return proper errors to NewContext function.
  call LogErrorAndExit from NewContext only by checking the errors and responding with proper wrapped message. This way we can properly handle errors from cluster(forbidden/timeout) and make it work for those commands that does not require cluster access.

Acceptance Criteria

• [ ] Unify Context functions to one function -> NewContext() It will also include detection of s2i or devfile component in context and return context accordingly.
• [x] Segregate functions into separate files, so that each utility function visible clearly. (at some places we are rewriting the code that is available as function in context package)
• [ ] Make sure some functionalities available only via context route e.g init config/envinfo, resolve app/project and remove their recurring occurrences
• [ ] Add Annotations to the commands that does not require cluster access and ignore client initialization error for them.
• [ ] Consolidate error handling at one place in the context package to have consistency in errors returned to the users.
• [ ] Move the localConfigProvider interface to some common package and extend it in way that all localConfig/Envinfo data get accessed through it.

[metacosm]

There shouldn't be any parameters for devfile/s2i/whatever: NewContext should do the detection automatically, the whole point is to isolate things so that clients don't need to know or care the type of components they're dealing with.

[adisky]

There shouldn't be any parameters for devfile/s2i/whatever: NewContext should do the detection automatically, the whole point is to isolate things so that clients don't need to know or care the type of components they're dealing with.

agreed, no parameters in NewContext autodetect s2i/devfile, requirement of cluster access and remove existing parameters as well ignoreMissingConfiguration, createAppIfNeeded

[adisky]

Document for context refactoring https://docs.google.com/document/d/1qiTTzR43jX_1igvw3oXtGJFjGVV7HeFRc8B_x37za90/edit#

[metacosm]

@adisky I've added some comments on the document, please also see: https://github.com/openshift/odo/issues/4057#issuecomment-717471798

[adisky]

@metacosm Thanks for your suggestions, I am trying to understand those

Pasting here the interface you suggested for context

package context

type Context interface {
	GetComponents() map[string]Component
	GetCurrentComponent() Component
	GetApplications() map[string]Application
	GetCurrentApplication() Application
	GetCurrentPath() string
	GetOptions() map[string]string // access command flags if needed
	GetClient() (Client, error)
	SetClient(client Client) error
}

don't you think it would be expensive to parse/evaluate with each call?

or you want to suggest, calculate and store once and give field access through functions?

type Context struct {
  application     string
  project            string
  client             Client
  options map[string] string
}

func GetContext(command  *cobra.Command) *Context {

}

func (context *Context)GetApplication() string{
}

func (context *Context)GetProject() string {
}

// all other options (e.g maxcpu, mincpu, storage for s2i or  e.g. runmode, pushcomand for devfile for devfile)
func(context *Context) GetOptions() map[string]string {

}

[metacosm]

@metacosm Thanks for your suggestions, I am trying to understand those

don't you think it would be expensive to parse/evaluate with each call?

What makes you think that things would be evaluated each time?

or you want to suggest, calculate and store once and give field access through functions?

The implementation of each function might depend on what needs to be done. That's the beauty of interfaces: you can do what you want and the calling code doesn't need to care. If you don't want to compute a value as soon as you create the underlying object, then you can defer until you actually need that value. If you can cache things, you can do it. If you need to re-compute things based on the current state of the application, you can also do it. None of these things can be done if your client code accesses the struct directly.

[adisky]

@metacosm Thanks for your suggestions, I am trying to understand those don't you think it would be expensive to parse/evaluate with each call?

What makes you think that things would be evaluated each time?

I got confused from your comments on the doc (taken not expose struct as not create) :)

[adisky]

Updated the acceptance criteria after the discussions and inputs from the team.

[dharmit]

• [ ] Add Annotations to the commands that does not require cluster access and ignore client initialization error for them.

@adisky Silly question alert - would this part help in indirectly addressing/fixing https://github.com/openshift/odo/issues/3779?

[adisky]

• [ ] Add Annotations to the commands that does not require cluster access and ignore client initialization error for them.

@adisky Silly question alert - would this part help in indirectly addressing/fixing #3779?

right it would fix those issues.

[adisky]

for this sprint

• [ ] Unify Context functions to one function -> NewContext() It will also include detection of s2i or devfile component in context and return context accordingly.
• [x] Segregate functions into separate files, so that each utility function visible clearly. (at some places we are rewriting the code that is available as function in context package)
• [ ] Make sure some functionalities available only via context route e.g init config/envinfo, resolve app/project and remove their recurring occurrences
• [ ] Add Annotations to the commands that does not require cluster access and ignore client initialization error for them.

[girishramnani]

I would suggest creating an interface called localConfigProvider that can span accross localConfig, envinfo and preference and then have a LocalComponentConfigProvider that spans only to localConfig and envinfo.

You can definitely observe the companility between localConfig, envInfo and preference package but as preference doesn't provide any component config and also there is nothing common in terms of the config values like name, project etc. so the localConfigProvider could provide filesystem IO and general config management which can then be build upon by LocalComponentConfigProvider maybe?

[adisky]

I would suggest creating an interface called localConfigProvider that can span accross localConfig, envinfo and preference and then have a LocalComponentConfigProvider that spans only to localConfig and envinfo.

You can definitely observe the companility between localConfig, envInfo and preference package but as preference doesn't provide any component config and also there is nothing common in terms of the config values like name, project etc. so the localConfigProvider could provide filesystem IO and general config management which can then be build upon by LocalComponentConfigProvider maybe?

Already mentioned about this in the acceptance criteria [ ] Move the localConfigProvider interface to some common package and extend it in way that all localConfig/Envinfo data get accessed through it.

[girishramnani]

The end goal is to provide a DRY common package that can be used by all three touch points to filesystem config. Maybe even consider viper? I feel it has many powerful features like live watching config file changes? https://github.com/spf13/viper

[adisky]

The end goal is to provide a DRY common package that can be used by all three touch points to filesystem config. Maybe even consider viper? I feel it has many powerful features like live watching config file changes? https://github.com/spf13/viper

Researched about viper a bit, regarding live watching config file changes i don't see any benefit for odo as it does not runs as a service to watch to config changes, w.r.t to odo viper is good tool to read/write from config files plus command line flags but i don't see any problem in our current implementation as well. Maybe I am missing something here, please explain little more if you think it would benefit us?

[adisky]

• [ ] Add Annotations to the commands that does not require cluster access and ignore client initialization error for them.

After working on this part i realized it is not possible to implement this on command level, e.g for odo catalog list components for s2i components we require cluster access and for devfile it is needed, for odo list inside context dir we do not require cluster access but outside we require, so this problem could not be solved using annotations at command level :|

[adisky]

Summarizing

• [ ] Unify Context functions to one function -> NewContext() It will also include detection of s2i or devfile component in context and return context accordingly. We need to think about this one if we are going to implement https://github.com/openshift/odo/issues/4281, as redirecting the s2i path to devfile the context might be the same for both.

• [x] Segregate functions into separate files, so that each utility function visible clearly. (at some places we are rewriting the code that is available as function in context package) This is done

• [ ] Make sure some functionalities available only via context route e.g init config/envinfo, resolve app/project and remove their recurring occurrences This needs to be done

• [ ] Add Annotations to the commands that does not require cluster access and ignore client initialization error for them. Stated the reason above, why it cannot be it cannot be done

• [ ] Consolidate error handling at one place in the context package to have consistency in errors returned to the users. This needs to be done

• [ ] Move the localConfigProvider interface to some common package and extend it in way that all localConfig/Envinfo data get accessed through it. This needs to be done

So finally following thing needs to be done

• [ ] Move the localConfigProvider interface to some common package and extend it in way that all localConfig/Envinfo data get accessed through it.
• [ ] Consolidate error handling at one place in the context package to have consistency in errors returned to the users.
• [ ] Make sure some functionalities available only via context route e.g init config/envinfo, resolve app/project and remove their recurring occurrences

[openshift-bot]

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-bot]

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-bot]

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[bot]]

@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.

[feloy]

Reopening the issue as more work would need to be done on Context refactoring

[dharmit]

/remove-lifecycle rotten

[feloy]

This has been resolved as part of the refactorings done in 2022/2023