Remove existing authentication and cart functionality (#2950) #2969

amarjandu · 2021-04-09T20:01:15Z

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)

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)

Started reindex in dev _{or this PR does not require reindexing dev}
Checked for failures in dev _{or this PR does not require reindexing dev}
Started reindex 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

codecov · 2021-04-09T20:24:32Z

Codecov Report

Merging #2969 (80ef6f8) into develop (8570300) will decrease coverage by 0.84%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2969      +/-   ##
===========================================
- Coverage    83.48%   82.64%   -0.85%     
===========================================
  Files          129      117      -12     
  Lines        13591    12168    -1423     
===========================================
- Hits         11347    10056    -1291     
+ Misses        2244     2112     -132

Impacted Files	Coverage Δ
src/azul/__init__.py	`82.27% <ø> (-0.79%)`	⬇️
src/azul/plugins/__init__.py	`89.91% <ø> (-0.09%)`	⬇️
src/azul/plugins/metadata/hca/__init__.py	`100.00% <ø> (ø)`
src/azul/service/elasticsearch_service.py	`81.42% <ø> (-0.67%)`	⬇️
src/azul/service/lambda_iam_policy.py	`0.00% <ø> (ø)`
test/app_test_case.py	`88.70% <ø> (-1.00%)`	⬇️
test/retorts.py	`100.00% <ø> (ø)`
test/service/test_request_builder.py	`92.06% <ø> (ø)`
src/azul/chalice.py	`98.66% <0.00%> (-1.34%)`	⬇️
src/azul/service/collection_data_access.py	`35.00% <0.00%> (-1.25%)`	⬇️
... and 1 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 8570300...80ef6f8. Read the comment docs.

coveralls · 2021-04-09T20:26:56Z

Coverage decreased (-0.3%) to 83.164% when pulling 80ef6f8 on issues/amar/2950-rm-auth into 8570300 on develop.

achave11-ucsc · 2021-04-12T21:49:18Z

Unassigning myself and assigning @noah-aviel-dove, as requested by
@hannes-ucsc.

achave11-ucsc

Great work. Only a weak observation, not sure if the changes would benefit from having multiple commits, like file/path renames and addition/deletions. However, since it is a refactor, it can be justified for the changes to be done in a single commit.

amarjandu · 2021-04-12T22:14:30Z

@achave11 I though about that a little, maybe it looks like having a commit history that looks like:

[1/3] Move entire service.app file to the attic
[2/3] Remove Auth/Cart segments out of the lambda/service/app
[3/3] Remove everything not related to the auth/cart stuff from the attic/lamdas/service/app

That might be a better way to track all the changes.

@noah-aviel-dove what you think about this?

nadove-ucsc · 2021-04-12T22:33:28Z

@noah-aviel-dove what you think about this?

Sounds a bit convoluted. I agree with @achave11 that the mixture of full rename/moves and partial additions/deletions does make the changes a little harder to track, but I think it's even harder to tell what's going on when the same files get touched in multiple commits for what is functionally a single change.

Having a [1/2] commit for moving entire files and and [2/2] commit for editing the unmoved files, or vice-versa, would be the clearest solution IMO. But a single commit also seems acceptable.

nadove-ucsc · 2021-04-12T22:45:53Z

attic/lambdas/service/app.py

@@ -0,0 +1,704 @@
+@app.authorizer()


I feel this file should preserve the import statements referenced in the moved code, otherwise we're not really preserving it.

Hard to tell whether this is correct advice or not without going every file in the attic. It just seems to me that there's a huge number of classes referenced in this code here that might be ambiguous to resolve in the future.

The main reason for the attic is not so much archival but maintaining a list of what used to exist. If you we just deleted files, it would be harder to find the commit that deleted a particular without knowing where it used to exist and what its name was.

To get to the exact original, one can always look at the commit that placed the file into the attic.

nadove-ucsc

Adding the import statements to the attic introduced conflicts. In light of @hannes-ucsc's clarifying comments, it seems better to omit the import statements from the attic after all.

nadove-ucsc · 2021-04-13T02:44:00Z

Additionally, there are several lingering references to the cart functionality throughout the codebase and TF templates:

~/PycharmProjects/azul-sc$ grep -i cart $(find src/azul/ -name '*.py')
src/azul/plugins/__init__.py:    cart_item: Mapping[str, Sequence[str]]
src/azul/plugins/metadata/hca/__init__.py:            cart_item={
src/azul/service/lambda_iam_policy.py:                f"arn:aws:dynamodb:{aws.region_name}:{aws.account}:table/{config.dynamo_cart_table_name}",
src/azul/service/lambda_iam_policy.py:                f"arn:aws:dynamodb:{aws.region_name}:{aws.account}:table/{config.dynamo_cart_item_table_name}",
src/azul/service/lambda_iam_policy.py:                f"arn:aws:dynamodb:{aws.region_name}:{aws.account}:table/{config.dynamo_cart_table_name}/index/*",
src/azul/service/lambda_iam_policy.py:                f"arn:aws:dynamodb:{aws.region_name}:{aws.account}:table/{config.dynamo_cart_item_table_name}/index/*",
src/azul/service/lambda_iam_policy.py:                f"arn:aws:states:{aws.region_name}:{aws.account}:stateMachine:{config.cart_item_state_machine_name}",
src/azul/service/lambda_iam_policy.py:                f"arn:aws:states:{aws.region_name}:{aws.account}:stateMachine:{config.cart_export_state_machine_name}"
src/azul/service/lambda_iam_policy.py:                f"arn:aws:states:{aws.region_name}:{aws.account}:execution:{config.cart_item_state_machine_name}:*",
src/azul/service/lambda_iam_policy.py:                f"arn:aws:states:{aws.region_name}:{aws.account}:execution:{config.cart_export_state_machine_name}"
src/azul/service/user_service.py:                                             item={'UserId': user_id, 'DefaultCartId': None})
src/azul/service/user_service.py:    def update(self, user_id: str, default_cart_id):
src/azul/service/user_service.py:        :param default_cart_id: the ID of the default cart (nullable)
src/azul/service/user_service.py:        if default_cart_id is not None:
src/azul/service/user_service.py:            update_conditions['DefaultCartId'] = None
src/azul/service/user_service.py:                                                    update_values={'DefaultCartId': default_cart_id},
src/azul/service/user_service.py:            raise UpdateError(user_id, default_cart_id)
src/azul/service/elasticsearch_service.py:    def transform_cart_item_request(self,
src/azul/service/elasticsearch_service.py:        Create a query using the given filter used for cart item requests
src/azul/service/elasticsearch_service.py:        :param entity_type: type of entities to write to the cart
src/azul/service/elasticsearch_service.py:        source_filter = list(chain(service_config.cart_item['bundles'],
src/azul/service/elasticsearch_service.py:                                   service_config.cart_item[entity_type]))
src/azul/__init__.py:    def dynamo_cart_table_name(self):
src/azul/__init__.py:        return self.qualified_resource_name('carts')
src/azul/__init__.py:    def dynamo_cart_item_table_name(self):
src/azul/__init__.py:        return self.qualified_resource_name('cartitems')
src/azul/__init__.py:    cart_item_write_lambda_basename = 'cartitemwrite'
src/azul/__init__.py:    def cart_item_state_machine_name(self):
src/azul/__init__.py:        return self.qualified_resource_name('cartitems')
src/azul/__init__.py:    def cart_export_max_batch_size(self):
src/azul/__init__.py:        return int(os.environ['AZUL_CART_EXPORT_MAX_BATCH_SIZE'])
src/azul/__init__.py:    def cart_export_min_access_token_ttl(self):
src/azul/__init__.py:        return int(os.environ['AZUL_CART_EXPORT_MIN_ACCESS_TOKEN_TTL'])
src/azul/__init__.py:    def cart_export_state_machine_name(self):
src/azul/__init__.py:        return self.qualified_resource_name('cartexport')
src/azul/__init__.py:    cart_export_dss_push_lambda_basename = 'cartexportpush'

It's not clear to me whether these fall under the scope of this ticket, since the title only mentioned the "cart service". But the (undocumented) UserService class and its associated unit tests only seem to be used by the attic-ed CartItemManager, so perhaps they ought to be attic-ed as well. Same for ElasticSearchService.transform_cart_item_request. I recommend discussing this with @hannes-ucsc at parking lot, unless he weighs in before then.

hannes-ucsc · 2021-04-13T17:13:03Z

The ticket speaks of "cart functionality", so yes, that stuff should be moved to attic as well. Consider collecting everything in a sub-directory of the attic e.g., attic/cart.

hannes-ucsc · 2021-04-16T16:25:35Z

UPGRADING.rst

+#2950 Move auth and cart service to attic
+=========================================
+
+1. Before upgrading to this commit, run ::


amarjandu · 2021-04-19T17:29:44Z

Force pushed branch after approval due to force push to develop branch.

achave11-ucsc · 2021-04-20T21:17:14Z

Unit test not passing in sandbox, reassigning to @amarjandu.

amarjandu · 2021-04-20T23:13:34Z

unable to replicate travis failure locally.
ran git prsq then pushed re-based changes to this PR branch.
tests now show passing.

github-actions bot added the orange [process] Done by the Azul team label Apr 9, 2021

amarjandu force-pushed the issues/amar/2950-rm-auth branch 5 times, most recently from 6ac79e3 to 59b913d Compare April 9, 2021 22:53

amarjandu assigned dsotirho-ucsc and achave11-ucsc and unassigned dsotirho-ucsc Apr 12, 2021

achave11-ucsc assigned nadove-ucsc and unassigned achave11-ucsc Apr 12, 2021

achave11-ucsc approved these changes Apr 12, 2021

View reviewed changes

nadove-ucsc requested changes Apr 12, 2021

View reviewed changes

nadove-ucsc removed their assignment Apr 12, 2021

amarjandu force-pushed the issues/amar/2950-rm-auth branch 2 times, most recently from ee9cc4e to 50c8150 Compare April 12, 2021 23:31

amarjandu assigned nadove-ucsc Apr 13, 2021

nadove-ucsc self-requested a review April 13, 2021 02:29

nadove-ucsc requested changes Apr 13, 2021

View reviewed changes

nadove-ucsc removed their assignment Apr 13, 2021

amarjandu force-pushed the issues/amar/2950-rm-auth branch 3 times, most recently from e19f7b8 to e1e6768 Compare April 14, 2021 15:01

amarjandu force-pushed the issues/amar/2950-rm-auth branch from b548b32 to d744515 Compare April 15, 2021 18:14

amarjandu assigned hannes-ucsc Apr 15, 2021

hannes-ucsc force-pushed the develop branch from 32e24e2 to 064f2c1 Compare April 15, 2021 20:07

hannes-ucsc mentioned this pull request Apr 16, 2021

Cycle error when removing AWS Lambda functions #2992

Closed

hannes-ucsc approved these changes Apr 16, 2021

View reviewed changes

hannes-ucsc removed their assignment Apr 16, 2021

hannes-ucsc added the no demo [process] Not to be demonstrated at the end of the sprint label Apr 16, 2021

hannes-ucsc assigned amarjandu Apr 16, 2021

hannes-ucsc changed the title ~~Move auth and cart service to attic (#2950)~~ Remove existing authentication and cart functionality (#2950) Apr 16, 2021

amarjandu force-pushed the issues/amar/2950-rm-auth branch from d744515 to 0376e84 Compare April 19, 2021 17:28

amarjandu assigned achave11-ucsc and unassigned amarjandu Apr 19, 2021

amarjandu added the upgrade [process] PR includes commit requiring manual upgrade label Apr 20, 2021

nadove-ucsc added the base [process] Another PR needs to be rebased before merging this one label Apr 20, 2021

achave11-ucsc force-pushed the issues/amar/2950-rm-auth branch from 0376e84 to d526156 Compare April 20, 2021 19:20

achave11-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Apr 20, 2021

achave11-ucsc removed their assignment Apr 20, 2021

achave11-ucsc removed the sandbox [process] Resolution is being verified in sandbox deployment label Apr 20, 2021

[u] Move auth and cart service to attic

80ef6f8

amarjandu force-pushed the issues/amar/2950-rm-auth branch from d526156 to 80ef6f8 Compare April 20, 2021 22:24

amarjandu assigned achave11-ucsc Apr 20, 2021

achave11-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Apr 21, 2021

achave11-ucsc merged commit 1bfaaf9 into develop Apr 21, 2021

achave11-ucsc deleted the issues/amar/2950-rm-auth branch April 21, 2021 15:46

achave11-ucsc removed their assignment Apr 21, 2021

nadove-ucsc mentioned this pull request Apr 21, 2021

Fix: Curl manifest expires after one hour (#2875) #2914

Merged

48 tasks

amarjandu mentioned this pull request Apr 26, 2021

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

Merged

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove existing authentication and cart functionality (#2950) #2969

Remove existing authentication and cart functionality (#2950) #2969

amarjandu commented Apr 9, 2021 •

edited by achave11-ucsc

Loading

codecov bot commented Apr 9, 2021 •

edited

Loading

coveralls commented Apr 9, 2021 •

edited

Loading

achave11-ucsc commented Apr 12, 2021

achave11-ucsc left a comment

amarjandu commented Apr 12, 2021

nadove-ucsc commented Apr 12, 2021

nadove-ucsc Apr 12, 2021

nadove-ucsc Apr 12, 2021

hannes-ucsc Apr 12, 2021

nadove-ucsc left a comment

nadove-ucsc commented Apr 13, 2021

hannes-ucsc commented Apr 13, 2021

hannes-ucsc Apr 16, 2021

amarjandu commented Apr 19, 2021

achave11-ucsc commented Apr 20, 2021 •

edited

Loading

amarjandu commented Apr 20, 2021

Remove existing authentication and cart functionality (#2950) #2969

Remove existing authentication and cart functionality (#2950) #2969

Conversation

amarjandu commented Apr 9, 2021 • edited by achave11-ucsc Loading

codecov bot commented Apr 9, 2021 • edited Loading

Codecov Report

coveralls commented Apr 9, 2021 • edited Loading

achave11-ucsc commented Apr 12, 2021

achave11-ucsc left a comment

Choose a reason for hiding this comment

amarjandu commented Apr 12, 2021

nadove-ucsc commented Apr 12, 2021

nadove-ucsc Apr 12, 2021

Choose a reason for hiding this comment

nadove-ucsc Apr 12, 2021

Choose a reason for hiding this comment

hannes-ucsc Apr 12, 2021

Choose a reason for hiding this comment

nadove-ucsc left a comment

Choose a reason for hiding this comment

nadove-ucsc commented Apr 13, 2021

hannes-ucsc commented Apr 13, 2021

hannes-ucsc Apr 16, 2021

Choose a reason for hiding this comment

amarjandu commented Apr 19, 2021

achave11-ucsc commented Apr 20, 2021 • edited Loading

amarjandu commented Apr 20, 2021

amarjandu commented Apr 9, 2021 •

edited by achave11-ucsc

Loading

codecov bot commented Apr 9, 2021 •

edited

Loading

coveralls commented Apr 9, 2021 •

edited

Loading

achave11-ucsc commented Apr 20, 2021 •

edited

Loading