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

Tests: Stage User Tracker implementation #210

Closed
wants to merge 2 commits into from

Conversation

gkaihorodova
Copy link
Contributor

@gkaihorodova gkaihorodova commented Nov 2, 2016

Fix provide possibility of creation stage user with minimal values,
with uid not specified. Implementation is the same as for User Tracker.

https://fedorahosted.org/freeipa/ticket/6448

@mirielka
Copy link
Contributor

Review notes: same as in #181

@martbab
Copy link
Contributor

martbab commented Dec 12, 2016

Bump for review.

@mirielka mirielka added the ack Pull Request approved, can be merged label Dec 15, 2016
@martbab
Copy link
Contributor

martbab commented Dec 15, 2016

Can we get this PR rebased against current master to see whether Travis CI is okay? The current checks are polluted by regression that should be fixed.

@stlaz stlaz removed the ack Pull Request approved, can be merged label Dec 16, 2016
stageduser_min.ensure_missing()
command = stageduser_min.make_create_command()
command()

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the indentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. Sorry for that.

if not (isinstance(givenname, six.string_types) and givenname):
raise ValueError("Invalid first name provided: %r" % givenname)
if not (isinstance(sn, six.string_types) and sn):
raise ValueError("Invalid second name provided: %r" % sn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use str.format() instead in new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will apply same for #181 as well

@stlaz
Copy link
Contributor

stlaz commented Dec 16, 2016

I just wanted to remove the ACK till @martbab's comment is worked in so nobody pushes it but I found some minor issues that I would like to see fixed in the rebase as well.

@gkaihorodova gkaihorodova force-pushed the fix-for-6448 branch 2 times, most recently from 941b477 to cab565d Compare January 6, 2017 11:09
@stlaz stlaz added the ack Pull Request approved, can be merged label Jan 9, 2017
@MartinBasti
Copy link
Contributor

Needs rebase

Applying: Unaccessible variable self.attrs in Tracker
Patch failed at 0001 Unaccessible variable self.attrs in Tracker
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: patch failed: ipatests/test_xmlrpc/tracker/base.py:76
error: ipatests/test_xmlrpc/tracker/base.py: patch does not apply

@martbab
Copy link
Contributor

martbab commented Jan 18, 2017

@gkaihorodova the PR cannot be pushed in current form because the first commit 298e1a1 is already pushed to master.

So to rebase it correctly please do the following:

Pull the changes from the remote repo (or any other label you have for it) into your local master branch:

$ git  checkout master; git pull

Then do the rebase against the refreshed master branch. The first commit should now disappear as git should detect that it is already there. If not, then abort the current rebase, re-start it in interactive mode (git rebase -i master) and remove the first commit manually (just remove the first line). Then force-push the changes into your fork:

$ git push -f origin fix-for-6448

Fix provide possibility of creation stage user with minimal values,
with uid not specified and check for non-empty unicode string
for attributes requested in init method

https://fedorahosted.org/freeipa/ticket/6448
Test to create stage user with minimal values, where uid is not specified

https://fedorahosted.org/freeipa/ticket/6448
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
5 participants