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

[R] Upgrade moto to 2.0.2 (#1718) #2886

Merged
merged 9 commits into from
May 21, 2021
Merged

Conversation

amarjandu
Copy link
Contributor

@amarjandu amarjandu commented Mar 10, 2021

Author

  • PR title references issue
  • Title of main commit references issue
  • PR is linked to Zenhub issue

Author (reindex)

  • Added r tag to commit title or this PR does not require reindexing
  • Added reindex label to PR or this PR does not require reindexing

Author (freebies & chains)

  • Freebies are blocked on this PR or there are no freebies in this PR
  • Freebies are referenced in commit titles or there are no freebies in this PR
  • This PR is blocked by previous PR in the chain or this PR is not chained to another PR
  • Added chain label to the blocking PR or this PR is not chained to another PR

Author (upgrading)

  • Documented upgrading of deployments in UPGRADING.rst or this PR does not require upgrading
  • Added u tag to commit title or this PR does not require upgrading
  • Added upgrade label to PR or this PR does not require upgrading

Author (requirements, before every review)

  • Ran make requirements_update or this PR leaves requirements*.txt, common.mk and Makefile untouched
  • Added R tag to commit title or this PR leaves requirements*.txt untouched
  • Added reqs label to PR or this PR leaves requirements*.txt untouched

Author (before every review)

  • make integration_test passes in personal deployment or this PR does not touch functionality that could break the IT
  • Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

  • Commented on demo expectations or labelled PR as no demo
  • Decided if PR can be labeled no sandbox
  • PR title is appropriate as title of merge commit
  • Moved ticket to Approved column
  • Assigned PR to an operator

Operator (before pushing merge the commit)

  • Checked reindex label and r commit title tag
  • Checked that Demo expectations are clear or PR is labeled as no demo
  • Rebased and squashed branch
  • Sanity-checked history
  • Pushed PR branch to Github
  • Branch pushed to Gitlab and build passes in sandbox or added no sandbox label
  • Started reindex in sandbox or this PR does not require reindexing sandbox
  • Checked for failures in sandbox or this PR does not require reindexing sandbox
  • Added PR reference to merge commit title
  • Collected commit title tags in merge commit title
  • Moved linked issue to Merged column
  • Pushed merge commit to Github

Operator (after pushing the merge commit)

  • Moved freebies to Merged column or there are no freebies in this PR
  • Shortened the PR chain or this PR is not the base of another PR
  • Verified that N reviews labelling is accurate
  • Pushed merge commit to Gitlab or this changes can be pushed later, together with another PR
  • Deleted PR branch from Github and Gitlab

Operator (reindex)

  • Purchased BQ slot committment in dev or this PR does not require reindexing dev
  • Started reindex in dev or this PR does not require reindexing dev
  • Deleted BQ slot committment in dev or this PR does not require reindexing dev
  • Checked for failures in dev or this PR does not require reindexing dev
  • Purchased BQ slot committment in prod or this PR does not require reindexing prod
  • Started reindex in prod or this PR does not require reindexing prod
  • Deleted BQ slot committment in prod or this PR does not require reindexing prod
  • Checked for failures in prod or this PR does not require reindexing prod

Operator

  • Unassigned PR

@github-actions github-actions bot added the orange [process] Done by the Azul team label Mar 10, 2021
@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch 7 times, most recently from b44a46e to 62f5f6a Compare March 17, 2021 01:26
@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch 2 times, most recently from 54d1659 to 129991a Compare March 17, 2021 18:21
@amarjandu amarjandu changed the title [r] Upgrade moto to 1.3.16 [R] Upgrade moto to 2.0.2 Mar 17, 2021
@amarjandu amarjandu changed the title [R] Upgrade moto to 2.0.2 [R] Upgrade moto to 2.0.2 (#1718) Mar 17, 2021
@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch 6 times, most recently from cf4372f to 192a3ac Compare March 22, 2021 18:06
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #2886 (0df9bc7) into develop (caa3711) will decrease coverage by 0.10%.
The diff coverage is 88.67%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2886      +/-   ##
===========================================
- Coverage    82.64%   82.54%   -0.11%     
===========================================
  Files          117      116       -1     
  Lines        12246    12180      -66     
===========================================
- Hits         10121    10054      -67     
- Misses        2125     2126       +1     
Impacted Files Coverage Δ
test/service/test_collection_data_access.py 20.35% <0.00%> (-8.22%) ⬇️
test/test_doctests.py 95.74% <ø> (-0.09%) ⬇️
test/version_table_test_case.py 94.44% <ø> (ø)
src/azul/portal_service.py 85.59% <80.00%> (-1.14%) ⬇️
src/azul/service/storage_service.py 90.71% <100.00%> (ø)
test/azul_test_case.py 71.64% <100.00%> (ø)
test/health_check_test_case.py 98.42% <100.00%> (-0.09%) ⬇️
test/indexer/test_hca_indexer.py 99.38% <100.00%> (-0.01%) ⬇️
test/indexer/test_indexer_controller.py 100.00% <100.00%> (ø)
test/service/test_drs.py 97.76% <100.00%> (-0.07%) ⬇️
... and 7 more

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 caa3711...0df9bc7. Read the comment docs.

@coveralls
Copy link

coveralls commented Mar 22, 2021

Coverage Status

Coverage decreased (-0.09%) to 83.077% when pulling 0df9bc7 on issues/amar/1718-moto-upgrade into caa3711 on develop.

@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch 2 times, most recently from abe3a90 to c854141 Compare March 22, 2021 18:57
@amarjandu amarjandu added the reqs [process] PR includes commit requiring ``make requirements`` label Mar 22, 2021
@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch 2 times, most recently from d1f156b to b9a9b90 Compare March 22, 2021 20:36
@amarjandu
Copy link
Contributor Author

amarjandu commented Mar 23, 2021

  • Verify sandbox IT passes
  • Push commit merges to github PR branch

@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch from 5ec6557 to ebb491a Compare March 24, 2021 02:19
@nadove-ucsc nadove-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Mar 24, 2021
@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch from 76786f6 to 3086ee3 Compare May 18, 2021 17:32
@hannes-ucsc hannes-ucsc force-pushed the issues/amar/1718-moto-upgrade branch from 3086ee3 to 4bc9384 Compare May 20, 2021 06:04
@hannes-ucsc hannes-ucsc removed their assignment May 20, 2021
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

See REVIEW commit. I did test the changes but they may have to be distributed over your existing commits. The change that removes the @contextmanager from _mock_service_endpoints should probably be a separate commit.

@hannes-ucsc hannes-ucsc force-pushed the issues/amar/1718-moto-upgrade branch from 4bc9384 to 8284ba7 Compare May 20, 2021 06:09
@hannes-ucsc
Copy link
Member

Alternatively, it's ok if you leave my REVIEW commit in place and just make it the 9/9 commit with a subtitle of like "Refactor response mock usage in health check tests`

@amarjandu
Copy link
Contributor Author

amarjandu commented May 20, 2021

Alternatively, it's ok if you leave my REVIEW commit in place and just make it the 9/9 commit with a subtitle of like "Refactor response mock usage in health check tests`

I'll update the commit titles and refresh the transitive dependencies.

note: Hannes REVIEW commit hash 8284ba7237b5b4c7dbf7de5b82755773f6c28f15

@amarjandu amarjandu force-pushed the issues/amar/1718-moto-upgrade branch 2 times, most recently from aab7cbc to 2a1cb25 Compare May 20, 2021 17:48
@hannes-ucsc hannes-ucsc added the no demo [process] Not to be demonstrated at the end of the sprint label May 20, 2021
@hannes-ucsc
Copy link
Member

I think the passing tests are demonstration enough. I know how hard this was and don't want to strip you of the opportunity to show this off. If you WANT to demo this, feel free to do so and take the no demo label off.

amarjandu and others added 9 commits May 20, 2021 16:12
Update requirements
Add FIXME (#3035)
Remove workarounds used in moto 1.3.13
Add StorageServiceTestCase
Remove ResponseHelper usage in tests
Fix create_table in version_table_test_case
Update botocore exception for portal_service
Add LocationConstraint to create_bucket in tests
Remove TestPortalService infra teardown
Refactor response mock usage in health check tests
@dsotirho-ucsc dsotirho-ucsc force-pushed the issues/amar/1718-moto-upgrade branch from e6f1269 to 0df9bc7 Compare May 20, 2021 23:13
@dsotirho-ucsc dsotirho-ucsc merged commit f0329d6 into develop May 21, 2021
@dsotirho-ucsc dsotirho-ucsc deleted the issues/amar/1718-moto-upgrade branch May 21, 2021 02:06
@dsotirho-ucsc dsotirho-ucsc removed their assignment May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4+ reviews [process] Lead requested changes four times or more no demo [process] Not to be demonstrated at the end of the sprint orange [process] Done by the Azul team reqs [process] PR includes commit requiring ``make requirements``
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants