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

Status checks if resource was upgraded #268

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nebril
Copy link
Contributor

@nebril nebril commented May 23, 2017

All resources now have a Definition to which they need to conform in
order to make an actual status check


This change is Reviewable

@nebril nebril force-pushed the resource-upgrade-status-picked branch from 2b5706a to 704d7ce Compare May 23, 2017 13:25
@istalker2
Copy link
Contributor

The commit mostly looks good. The problem is that it adds new status and functionality which is not used by deployment engine (scheduler). Thus it is not clear, how it is going to be used. At least it should be reflected in commit message. Without understanding the purpose it is hard to review the code. My guess is that from now on definitions are going to be the single point of truth and if, for example, I want to set resource label I need to modify definition and rerun the graph rather than doing it through kubectl, as usual


Review status: 0 of 30 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


pkg/interfaces/interfaces.go, line 43 at r1 (raw file):

	Delete() error
	Meta(string) interface{}
	EqualToDefinition(interface{}) bool

Shouldn't it be EqualsToDefinition?
Also, correct me if I'm wrong, but resources only use this method in their Status() implementation. If so, why is it part of the resource interface?


pkg/resources/configmap.go, line 102 at r1 (raw file):

	if !c.EqualToDefinition(cm) {
		return interfaces.ResourceWaitingForUpgrade, fmt.Errorf(string(interfaces.ResourceWaitingForUpgrade))

Here and in other resources: why it it error? Usually it is either result and nil error, or nil and non-nil error. If there is a special status for this then resource being in that status is not an error condition. Error is when you cannot get the status, for example


Comments from Reviewable

@nebril
Copy link
Contributor Author

nebril commented May 24, 2017

Seems like some of the scheduling code got lost during rebase (it was long and painful). Will fix. Thanks for the feedback, will try to fix.

All resources now have a Definition to which they need to conform in
order to make an actual status check
@nebril nebril force-pushed the resource-upgrade-status-picked branch from 704d7ce to b4e665b Compare May 25, 2017 13:07
@nebril
Copy link
Contributor Author

nebril commented May 25, 2017

Review status: 0 of 28 files reviewed at latest revision, 2 unresolved discussions.


pkg/interfaces/interfaces.go, line 43 at r1 (raw file):

Previously, istalker2 (Stan Lagun) wrote…

Shouldn't it be EqualsToDefinition?
Also, correct me if I'm wrong, but resources only use this method in their Status() implementation. If so, why is it part of the resource interface?

Done.


pkg/resources/configmap.go, line 102 at r1 (raw file):

Previously, istalker2 (Stan Lagun) wrote…

Here and in other resources: why it it error? Usually it is either result and nil error, or nil and non-nil error. If there is a special status for this then resource being in that status is not an error condition. Error is when you cannot get the status, for example

Done.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants