Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Reduction of information in response to operation postponed #3631

Merged
merged 1 commit into from Sep 26, 2018
Merged

Reduction of information in response to operation postponed #3631

merged 1 commit into from Sep 26, 2018

Conversation

vdusek
Copy link

@vdusek vdusek commented Sep 13, 2018

@pep8speaks
Copy link

Hello @vdusek! Thanks for submitting the PR.

@pulpbot
Copy link
Member

pulpbot commented Sep 13, 2018

Can one of the admins verify this patch?

@daviddavis
Copy link
Contributor

LGTM. You have some test failures though.

@daviddavis
Copy link
Contributor

Also, we should probably update all the READMEs. Here's pulp_file for example:

https://github.com/pulp/pulp_file/blob/master/README.rst#use-the-bar-publisher-to-create-a-publication

@pulpbot
Copy link
Member

pulpbot commented Sep 14, 2018

Can one of the admins verify this patch?

@vdusek
Copy link
Author

vdusek commented Sep 14, 2018

One of the solution is rename task field back to _href, as it's done now.

The second solution is to leave field named task and edit the tests. Specifically edit keys from '_href' to 'task' here - pulp_smash/api.py#L505 and here - pulpcore/tests/functional/api/test_tasks.py#L44. However it could seem kind of confusing from the perspective of creating tests, because key '_href' is used almost everywhere.

What do you think @daviddavis, @dkliban?

@daviddavis
Copy link
Contributor

I lean towards updating the tests.

Also, ping @pulp/qe for their thoughts.

@vdusek
Copy link
Author

vdusek commented Sep 14, 2018

Updated and opened PR on pulp-smash - pulp-smash/pull/1137.

@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #3631 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3631   +/-   ##
=======================================
  Coverage   55.79%   55.79%           
=======================================
  Files          63       63           
  Lines        2690     2690           
=======================================
  Hits         1501     1501           
  Misses       1189     1189
Impacted Files Coverage Δ
pulpcore/pulpcore/app/response.py 66.66% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa9e27d...ea24dd9. Read the comment docs.

@vdusek
Copy link
Author

vdusek commented Sep 26, 2018

I added to the commit message reference to the required PR from pulp-smash, according docs (thanks @dkliban). I updated and rebased both PRs, just for sure, but travis-ci still didn't pass. So please, if anyone could tell me where's the problem or what I've been doing wrong, I'll be glad. Because locally checks pass without any trouble.

======================================== short test summary info ========================================
SKIP [1] tests/functional/api/test_auth.py:57: https://pulp.plan.io/issues/3248
SKIP [1] tests/functional/api/test_auth.py:48: https://pulp.plan.io/issues/3248
SKIP [1] tests/functional/api/test_auth.py:108: https://pulp.plan.io/issues/3248
SKIP [1] tests/functional/api/test_auth.py:94: https://pulp.plan.io/issues/3248

================================ 88 passed, 4 skipped in 317.55 seconds =================================

@daviddavis
Copy link
Contributor

daviddavis commented Sep 26, 2018

It looks we removed the functionality that lets you require pulp-smash PRs when we moved the pulp-smash tests into the pulp repo. I think we need to merge the pulp-smash change first and then re-run this PR.

Edit: I opened an issue https://pulp.plan.io/issues/4037

@daviddavis daviddavis merged commit 2af1a22 into pulp:master Sep 26, 2018
@vdusek vdusek deleted the issue_3978 branch September 27, 2018 06:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants