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

Removed incorrect check for returncode #52

Closed
wants to merge 4 commits into from
Closed

Removed incorrect check for returncode #52

wants to merge 4 commits into from

Conversation

ofayans
Copy link
Contributor

@ofayans ofayans commented Sep 2, 2016

The server installation in most cases returns response code 0 no matter what
happens except for really severe errors. In this case when we try to uninstall
the middle replica of a line topology, it fails, notifies us that we should use
'--ignore-topology-disconnect', but returns 0

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

@@ -1190,7 +1190,7 @@ def run_server_del(host, server_to_delete, force=False,
def assert_error(result, stderr_text, returncode=None):
"Assert that `result` command failed and its stderr contains `stderr_text`"
assert stderr_text in result.stderr_text, result.stderr_text
if returncode:
if returncode is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

@MartinBasti
Copy link
Contributor

ACK: Removed incorrect check for returncode
NACK: Several fixes in replica_promotion tests -- missing explanation in commit messages why this change is needed, if changes are unrelated should be in multiple patches
ACK: Changed addressing to the client hosts to be replicas
ACK: Xfailed the tests due to a known bug with replica preparation

@ofayans
Copy link
Contributor Author

ofayans commented Sep 5, 2016

The commit message was updated

@MartinBasti MartinBasti added ack Pull Request approved, can be merged and removed ack Pull Request approved, can be merged labels Sep 5, 2016
@MartinBasti
Copy link
Contributor

I went through commit messages again and I changed my mind, NACK:

commit: Removed incorrect check for returncode

This commit contains incorrect ticket, issue has not been fixed by this commit

commit: Several fixes in replica_promotion tests

This does not have ticket, is okay to push it just to master?

All commits have different tickets, so I have to manually split hashes and put it to proper tickets. Leave it as is for this PR to avoid mess, but in future create one PR for one ticket

The server installation in most cases returns response code 0 no matter what
happens except for really severe errors. In this case when we try to uninstall
the middle replica of a line topology, it fails, notifies us that we should use
'--ignore-topology-disconnect', but returns 0

https://fedorahosted.org/freeipa/ticket/6300
@ofayans
Copy link
Contributor Author

ofayans commented Sep 6, 2016

@mbasti-rh,

  1. Fixed
  2. It's OK, but we won't have working tests in 4.3 branch. Should I create a ticket?
  3. Gotya :)

@MartinBasti
Copy link
Contributor

Ad 2. yes

Oleg Fayans added 3 commits September 6, 2016 13:39
In test_one_command_installation the ipa-replica-install was missing '--server'
and '-U' options which resulted in false negative result. In
test_client_enrollment_by_unprivileged_user '--server' option was messing.
test_replica_promotion_after_adding_to_admin_group lacked '-U' option. It
leaded to 3 failed cases.

https://fedorahosted.org/freeipa/ticket/6301
@ofayans
Copy link
Contributor Author

ofayans commented Sep 6, 2016

Done

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Sep 14, 2016
@MartinBasti
Copy link
Contributor

@MartinBasti
Copy link
Contributor

@MartinBasti
Copy link
Contributor

Xfailed the tests due to a known bug with replica preparation

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/1e484d010b653b8ac06425ca602ba5c9e950ed89

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Sep 14, 2016
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
2 participants