-
Notifications
You must be signed in to change notification settings - Fork 243
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
Refactoring Context #4081
Comments
There shouldn't be any parameters for devfile/s2i/whatever: |
agreed, no parameters in |
Document for context refactoring https://docs.google.com/document/d/1qiTTzR43jX_1igvw3oXtGJFjGVV7HeFRc8B_x37za90/edit# |
@adisky I've added some comments on the document, please also see: #4057 (comment) |
@metacosm Thanks for your suggestions, I am trying to understand those Pasting here the interface you suggested for context
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?
|
What makes you think that things would be evaluated each time?
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. |
I got confused from your comments on the doc (taken not expose struct as not create) :) |
Updated the acceptance criteria after the discussions and inputs from the team. |
for this sprint
|
After working on this part i realized it is not possible to implement this on command level, e.g for |
Summarizing
So finally following thing needs to be done
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closing this issue. In response to this:
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. |
Reopening the issue as more work would need to be done on Context refactoring |
/remove-lifecycle rotten |
This has been resolved as part of the refactorings done in 2022/2023 |
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
fromNewContext
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
It will also include detection of s2i or devfile component in context and return context accordingly.
The text was updated successfully, but these errors were encountered: