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.

Sign up for GitHub

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

Jump to bottom

Fixes #1158: mofcomp rollback of test.mof fails #1944

Merged
merged 1 commit into from Nov 29, 2019
Merged

Fixes #1158: mofcomp rollback of test.mof fails #1944

merged 1 commit into from Nov 29, 2019

Conversation

KSchopmeyer
Copy link
Collaborator

@KSchopmeyer KSchopmeyer commented Nov 10, 2019 •
edited

Fixes issue with mof_compiler and rolback. See commit for details

@KSchopmeyer KSchopmeyer added the roll back needed Roll back into latest fix branch is needed label Nov 10, 2019
@KSchopmeyer KSchopmeyer added this to the 1.0.0 milestone Nov 10, 2019
@KSchopmeyer KSchopmeyer self-assigned this Nov 10, 2019
@coveralls
Copy link

coveralls commented Nov 10, 2019 •
edited

Coverage Status

Coverage increased (+0.2%) to 86.224% when pulling eeff2d4 on ks/#1158-repo-remove-fails into a06bf72 on master.

@andy-maier andy-maier changed the title WIP Fixes issue #1158 mofcomp remove of test.mof fails [WIP] Fixes #1158: mofcomp remove of test.mof fails Nov 11, 2019
This was referenced Nov 11, 2019
Closed
Closed
@andy-maier
Copy link
Contributor

Please rebase since PR #1946 which fixes the Appveyor issue has been merged.

@andy-maier andy-maier changed the title [WIP] Fixes #1158: mofcomp remove of test.mof fails Fixes #1158: mofcomp remove of test.mof fails Nov 18, 2019
@andy-maier andy-maier changed the title Fixes #1158: mofcomp remove of test.mof fails [WIP] Fixes #1158: mofcomp remove of test.mof fails Nov 18, 2019
@andy-maier andy-maier requested review from andy-maier and removed request for andy-maier November 18, 2019 21:52
@andy-maier
Copy link
Contributor

Suggest to rebase it, because another PR was merged that changes this area.

@andy-maier andy-maier removed their request for review November 20, 2019 12:23
@KSchopmeyer KSchopmeyer changed the title [WIP] Fixes #1158: mofcomp remove of test.mof fails Fixes #1158: mofcomp remove of test.mof fails Nov 22, 2019
@andy-maier andy-maier requested review from andy-maier and removed request for andy-maier November 24, 2019 04:35
andy-maier
andy-maier requested changes Nov 24, 2019
View reviewed changes
Copy link
Contributor

@andy-maier andy-maier left a comment

Choose a reason for hiding this comment

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

Needs to wait for resolution of comments on PR #1957.

@andy-maier
Copy link
Contributor

andy-maier commented Nov 28, 2019 •
edited by KSchopmeyer

Could you please rebase on master?

COMMENT: KS It is up-to-date with master.

andy-maier
andy-maier requested changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@andy-maier andy-maier left a comment •
edited by KSchopmeyer

Choose a reason for hiding this comment

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

You say the PR is up to date with master, and while that is true, it still contains the commits of PR #1957 even though that PR has been merged into master. The content of these commit is different from what was merged into master, but the overall content changes of the combined four commits looks good to me.

I could deal with that by squashing these commits into one and deleting the commit messages of PR #1957, but for one, the author should be doing that and not the merger, and more importantly, there is nothing meaningful left after deleting them because the other two commits that actually fix #1158 have no meaningful commit text.

A second problem I found by looking at the single changes of the two commits from PR #1957 is that they introduce merge conflict markers, which get fixed by the the two commits. Again, the overall content changes of the combined four commits looks good to me, so the issue is only with the number of commits and the commit messages.

So please squash these commits into one, and give it a meaningful commit message.

COMMENT: All commits squashed to the single commit I want. Added test of rollback both with manual test and in test_mofcompiler.py. Added documentation on limitations as I know them today.

@KSchopmeyer
Fixes #1158: mofcomp rollback of test.mof with mof_compiler fails
eeff2d4 
Fixes issue in mof_compiler where the remove option fails to remove
mof created by an earlier compile of the same file. This was
primarily an issue of getting the instance paths correct on the
CreateInstance.

Adds a test for rollbacik to test_mofcompiler.py based on using the
mocker and test.mof

Adds a second manual test against a server at http://localhost
that executes mof_compiler to create and rollback multiple times
in two namespaces.

Adds documentation on the rollback limitations to the rollback
method documentation.
@KSchopmeyer KSchopmeyer changed the title Fixes #1158: mofcomp remove of test.mof fails Fixes #1158: mofcomp rollback of test.mof fails Nov 29, 2019
@andy-maier andy-maier mentioned this pull request Nov 29, 2019
Merged
@andy-maier andy-maier removed the roll back needed Roll back into latest fix branch is needed label Nov 29, 2019
@andy-maier andy-maier merged commit 2f28dd9 into master Nov 29, 2019
@andy-maier andy-maier deleted the ks/#1158-repo-remove-fails branch November 29, 2019 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-maier andy-maier andy-maier approved these changes

Assignees

@KSchopmeyer KSchopmeyer

Labels
None yet
Projects
None yet
Milestone
1.0.0b1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@KSchopmeyer @coveralls @andy-maier