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

Improve tag mapping specs #170

Merged
merged 1 commit into from
Nov 19, 2017
Merged

Conversation

cben
Copy link
Contributor

@cben cben commented Nov 16, 2017

Split from #162

@cben
Copy link
Contributor Author

cben commented Nov 16, 2017

@miq-bot add-label test, gaprindashvili/yes

@zeari @moolitayer Please review

@cben cben mentioned this pull request Nov 16, 2017
23 tasks
@moolitayer
Copy link

@cben so this change is testing that additional tags that did not come back in refresh are not removed?
Do we always not remove tags that did not come back in refresh? if not, In which cases do we not delete them?

@cben
Copy link
Contributor Author

cben commented Nov 16, 2017

@moolitayer This tests not removing a user-defined user-assigned tag.
It's assigned after 1st refresh because the object must exist for user to do Edit Tags.

We should of course be removing mapping-generated mapping-assigned tags, when the label disappears.
That will be tested in similar openshift PR I'm sending soon, where we such situation in the cassette.

@cben cben force-pushed the tag-mapping-tests branch from 4f7d2e6 to 54595b6 Compare November 19, 2017 13:23
- Use factory for more accurate test.
  Previous test didn't set read_only nor used 'kubernetes:' prefix,
  will fail with upcoming retagging implementation.

- Test that we don't remove user-assigned tags.
@cben cben force-pushed the tag-mapping-tests branch from 54595b6 to 5b594f3 Compare November 19, 2017 13:39
@cben
Copy link
Contributor Author

cben commented Nov 19, 2017

Fixed potential double tagging on 3rd refresh.
Rewrote for clearer data flow by arg passing, unrolled the loop, improved comment (I hope).
PTAL.

@miq-bot
Copy link
Member

miq-bot commented Nov 19, 2017

Checked commit cben@5b594f3 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆


# Now records exist, simulate user assigning tags by Edit Tags, to test later refreshes don't remove them.
@replicator.reload.tags |= [@user_tag]
@containergroup.reload.tags |= [@user_tag]

Choose a reason for hiding this comment

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

I might be missing something, but if the refresh does remove the user tags won't this code re set them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. This was a tricky point with the loop but is simpler now it's unrolled:
here is the one and only time we set the user tag.
next 2 full refreshes only perform refresh, and assert the tag is still assigned.

Do you see any clearer way to write this? I'm really happy with the structure I reached currently, but what matters is somebody else being able to understand...

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@replicator.reload.tags |= [@user_tag] means
@replicator.reload.tags = @replicator.reload.tags | [@user_tag] right?
do we reload here twice?

Merging and we can reiterate if needed, thanks!

@moolitayer moolitayer merged commit ef60398 into ManageIQ:master Nov 19, 2017
@moolitayer moolitayer added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 19, 2017
simaishi pushed a commit that referenced this pull request Nov 20, 2017
Improve tag mapping specs
(cherry picked from commit ef60398)
@simaishi
Copy link

Gaprindashvili backport details:

$ git log -1
commit d2e40c479ac04aa557d7b714dbab3f6827b3cd52
Author: Mooli Tayer <[email protected]>
Date:   Sun Nov 19 23:00:20 2017 +0200

    Merge pull request #170 from cben/tag-mapping-tests
    
    Improve tag mapping specs
    (cherry picked from commit ef6039818308c25baab8abadaf124aa28952e108)

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.

5 participants