Replace ProgressBar with ProgressReport #293
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bmbouter
reviewed
Sep 17, 2019
| @@ -386,17 +386,6 @@ class Migration(migrations.Migration): | |||
| }, | |||
| bases=('core.progressreport',), | |||
| ), | |||
| migrations.CreateModel( | |||
There was a problem hiding this comment.
Can we apply this as a new migration instead? That will allow us to keep pre-production upgrades simpler.
fao89
force-pushed
the
5164
branch
2 times, most recently
from
e6d9936
to
ba0b10e
Compare
|
This PR started as a bugfix, but then it transitioned more into doing the work of 5444 which I believe is right because we want to get the big parts right and then handle bugs on them. Can this PR become dedicated to 5444 and not include the bugfix? My concern is that we would be spreading out the 5444 (possible breaking change) across more PRs and time instead of all at once. What do you think? |
fao89
added a commit
to fao89/pulp_file
that referenced
this pull request
Sep 17, 2019
ref #5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127
fao89
added a commit
to fao89/pulpcore-plugin
that referenced
this pull request
Sep 17, 2019
ref #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulpcore-plugin
that referenced
this pull request
Sep 17, 2019
ref #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulpcore-plugin
that referenced
this pull request
Sep 17, 2019
ref #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_file
that referenced
this pull request
Sep 17, 2019
ref #5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127
bmbouter
reviewed
Sep 17, 2019
CHANGES/5444.misc
Outdated
| @@ -0,0 +1 @@ | |||
| Using `ProgressReport` for known and unknown items count. | |||
There was a problem hiding this comment.
This would be good as a .feature so it can be shown (because of the misc template not being shown).
bmbouter
reviewed
Sep 17, 2019
pulpcore/app/models/progress.py
Outdated
|
|
||
| class Meta: | ||
| proxy = True | ||
|
|
||
| def increment(self): | ||
| """ | ||
| Increment done count and save the progress bar. |
There was a problem hiding this comment.
Can progress bar be updated please.
fao89
added a commit
to fao89/pulpcore-plugin
that referenced
this pull request
Sep 17, 2019
ref #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulp_file#277
bmbouter
reviewed
Sep 17, 2019
| @@ -31,8 +31,7 @@ class ProgressReport(Model): | |||
| code (models.CharField): identifies the type of progress report | |||
There was a problem hiding this comment.
Can L#25 be updated since it's no longer a base model, but to be used directly.
bmbouter
reviewed
Sep 17, 2019
pulpcore/app/models/progress.py
Outdated
| @@ -106,8 +105,8 @@ def __exit__(self, type, value, traceback): | |||
| See the context manager documentation for more info on __exit__ parameters | |||
| """ | |||
| self._using_context_manager = False | |||
| if self.total is None and self.done != 0: | |||
| self.total = self.done | |||
| if self.total is None: | |||
There was a problem hiding this comment.
I'm not sure why we have this conditional case. Can we do away with this part?
fao89
changed the title
|
Since we need to redo migrations for this change anyways, I decided to make another small change that would also require redoing the migrations, and we can merge them around the same time to avoid doing it more than once. |
fao89
added a commit
to fao89/pulp-2to3-migration
that referenced
this pull request
Sep 18, 2019
ref #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
This was referenced Sep 18, 2019
fao89
added a commit
to fao89/pulp_deb
that referenced
this pull request
Sep 18, 2019
closes #5477 https://pulp.plan.io/issues/5477 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_rpm
that referenced
this pull request
Sep 18, 2019
closes #5469 https://pulp.plan.io/issues/5469 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_python
that referenced
this pull request
Sep 18, 2019
closes #5471 https://pulp.plan.io/issues/5471 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_docker
that referenced
this pull request
Sep 18, 2019
closes #5470 https://pulp.plan.io/issues/5470 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
force-pushed
the
5164
branch
2 times, most recently
from
ab00f33
to
cd309c3
Compare
fao89
added a commit
to fao89/pulp-2to3-migration
that referenced
this pull request
Sep 19, 2019
ref #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
mdellweg
reviewed
Sep 19, 2019
| @@ -20,8 +20,6 @@ | |||
|
|
|||
| class ProgressReport(Model): | |||
| """ | |||
| A base model for all progress reporting. | |||
There was a problem hiding this comment.
I would keep that line and just remove the 'base'.
mdellweg
reviewed
Sep 19, 2019
| You can also use this short form which handles all necessary save() calls: | ||
|
|
||
| >>> with ProgressSpinner(message='Publishing Metadata'): | ||
| >>> publish_metadata() |
There was a problem hiding this comment.
It's a pity, that we loose all those good code examples.
There was a problem hiding this comment.
I agree and didn't realize that. @fabricio-aguiar can we move these back to the docstrings we are keeping?
mdellweg
reviewed
Sep 19, 2019
| share the same in-memory instance. Django does not synchronize in-memory model instances, so | ||
| multiple instances of a specific ProgressBar will diverge as they are written to from separate | ||
| threads. | ||
| """ |
There was a problem hiding this comment.
... I think the documentation should transmogrified into the foremer base model.
daviddavis
pushed a commit
to daviddavis/pulp_ansible
that referenced
this pull request
Sep 19, 2019
Required PR: pulp/pulpcore#293 [noissue]
daviddavis
pushed a commit
to daviddavis/pulp_ansible
that referenced
this pull request
Sep 19, 2019
Required PR: pulp/pulpcore#293 [noissue]
daviddavis
pushed a commit
to daviddavis/pulp_ansible
that referenced
this pull request
Sep 19, 2019
Required PR: pulp/pulpcore#293 [noissue]
closes #5444 https://pulp.plan.io/issues/5444 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_rpm
that referenced
this pull request
Sep 19, 2019
closes #5469 https://pulp.plan.io/issues/5469 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_python
that referenced
this pull request
Sep 19, 2019
closes #5471 https://pulp.plan.io/issues/5471 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_docker
that referenced
this pull request
Sep 19, 2019
closes #5470 https://pulp.plan.io/issues/5470 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_rpm
that referenced
this pull request
Sep 19, 2019
closes #5469 https://pulp.plan.io/issues/5469 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
bmbouter
approved these changes
Sep 19, 2019
daviddavis
approved these changes
Sep 19, 2019
fao89
added a commit
to fao89/pulp_docker
that referenced
this pull request
Sep 20, 2019
closes #5470 https://pulp.plan.io/issues/5470 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
fao89
added a commit
to fao89/pulp_rpm
that referenced
this pull request
Sep 20, 2019
closes #5469 https://pulp.plan.io/issues/5469 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
ThikaXer
pushed a commit
to ATIX-AG/pulp_deb
that referenced
this pull request
Oct 5, 2020
closes #5477 https://pulp.plan.io/issues/5477 Required PR: pulp/pulpcore#293 Required PR: pulp/pulpcore-plugin#127 Required PR: pulp/pulp_file#277
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #5444
Required PR: pulp/pulpcore-plugin#127
https://pulp.plan.io/issues/5444
Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/en/3.0/nightly/contributing/pull-request-walkthrough.html