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

crowbar: Remove forgotten nodes from proposals #1116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vuntz
Copy link
Member

@vuntz vuntz commented Feb 20, 2017

Forgotten nodes are not removed from existing proposals (applied and
non-applied). While the webui removes non-existing nodes when displaying
a proposal, if only the API is used, applying a proposal with such
non-existing nodes may result in failures.

Closes sap-oc#8

@@ -132,7 +132,32 @@ def transition(inst, name, state)
end

if state == "delete"
# Do more work here - one day.
# Remove node from all applied proposals
RoleObject.all.select(&:proposal?).each do |role|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - role.

Forgotten nodes are not removed from existing proposals (applied and
non-applied). While the webui removes non-existing nodes when displaying
a proposal, if only the API is used, applying a proposal with such
non-existing nodes may result in failures.

Closes sap-oc#8
@matelakat
Copy link
Contributor

@vuntz shall we merge this, or the CI failure is a blocker?

@matelakat
Copy link
Contributor

@aspiers could you weigh in please, we need your help :-)

@aspiers
Copy link
Member

aspiers commented Jun 15, 2017

The CI failure is a standard haproxy failure which has been happening for a while. I doubt the CI even exercises this code path.

Copy link
Member

@aspiers aspiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought hard about this, but it feels like there might be one or even two privacy violations here. I'm sure that's typical within Crowbar, but while fixing them is out of scope of this PR, maybe it shouldn't add new ones.

The first one would be directly manipulating RoleObjects which represent proposals, rather than going through a Proposal facade. Secondly, removing nodes from proposals seems like something that should belong to Proposal, not here. I'm inclined to think that DeployerService should simply invoke that API, rather than implement the removal itself.

However there's a good chance I'm way off base here, in which case let me know and I'll remove my -1.

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

Successfully merging this pull request may close these issues.

Forgotten nodes are still referenced in proposals
8 participants