Isolation segments cleanup #723
Conversation
Signed-off-by: Sandy Cash <scarlet.tanager@gmail.com>
…egmentAssign actions [#133869287] Signed-off-by: Dan Lavine <dlavine@us.ibm.com>
- add shared segment equality method [#133869287] Signed-off-by: Dan Lavine <dlavine@us.ibm.com>
from the Space controller to the Space model. [#133869287] Signed-off-by: Dan Lavine <dlavine@us.ibm.com>
[#133869287] Signed-off-by: Sandy Cash <scarlet.tanager@gmail.com>
…for IS <-> orgs [#133869287] Signed-off-by: Dan Lavine <dlavine@us.ibm.com>
[#133869287] Signed-off-by: Dan Lavine <dlavine@us.ibm.com>
- Use IsolationSegmentModel.is_shared_segment? - Fix specs after rebase - Rubocop - Allow org.default_isolation_segment_model to go from nil to shared [finishes #133869287] Signed-off-by: Sandy Cash <scarlet.tanager@gmail.com>
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/134468621 The labels on this github issue will be updated when the story is started. |
|
Hey ScarletTanager! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
| @@ -8,14 +8,22 @@ module VCAP::CloudController | |||
| let(:org2) { Organization.make } | |||
|
|
|||
| it 'sorts the organizations passed in for assignment' do | |||
| org.update(guid: 'b') | |||
| org2.update(guid: 'a') | |||
| rval = Random.new.rand(1..2) | |||
There was a problem hiding this comment.
I'd prefer having two tests with the two scenarios instead of one test that randomly picks one.
|
|
||
| org.reload | ||
| org2.reload | ||
| if rval == 1 |
There was a problem hiding this comment.
Same as above about random tests.
|
|
||
| context 'associating an isolation_segment' do | ||
| context 'when the space contains no apps' do |
There was a problem hiding this comment.
We're curious about what drove us to remove these tests. Are they no longer necessary? Are we covering these scenarios somewhere else?
There was a problem hiding this comment.
@luan Yes, @DanLavine and I moved these into the model spec for space, since the check is now in a validation in that class.
There was a problem hiding this comment.
@ScarletTanager We were able to find the tests for when spaces contains apps, but we were unable to find the tests for spaces that had no apps in them, particularly testing associating the space with the isolation segment as different roles. Could you point us to where those tests landed?
There was a problem hiding this comment.
@thausler786 What is the difference in the space having apps or not when assigning an Isolation Segment? If the user is unable to perform the action, then they will get a 403 regardless. Those tests are still in this spaces_controller_spec.rb file.
There was a problem hiding this comment.
@SocalNick I'm not sure who's in the queue to pick this up, but hopefully we've answered the question here?
There was a problem hiding this comment.
Community pair changes every day. Maybe ping us in Slack to see if you can get some realtime confirmation during working hours today.
| 'Adding the Isolation Segment to the Space', | ||
| "Only Isolation Segments in the Organization's allowed list can be used.") | ||
| end | ||
| raise CloudController::Errors::ApiError.new_from_details('ResourceNotFound', 'Isolation Segment not found') if |
There was a problem hiding this comment.
This is a little confusing to read. It may be the go developer talking but I don't really like the if at the end of the line with no conditions. If you were to merge the condition in the same line it'd be a really long line. So I'd prefer if this was just a regular if around the raise.
- Replace randomized sort tests with individual tests for each possible order - Reformat conditional raise [#133869287] Signed-off-by: Dan Lavine <dlavine@us.ibm.com>
|
@luan - Pushed a commit with the requested changes. Thanks! |
Story reference https://www.pivotaltracker.com/story/show/133869287