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

[RFC] Remove COMP/CON Authentication #706

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[RFC] Remove COMP/CON Authentication #706

wants to merge 2 commits into from

Conversation

BoltsJ
Copy link
Collaborator

@BoltsJ BoltsJ commented Jun 21, 2024

Proposed removal of COMP/CON login for pilot importing

Reasons we might want to remove this feature

  • Maintenance: Keeping the aws dependencies updated represents a non-trivial amount of work.
  • Security: There are several current security notices for this repo due to aws-amplify and its dependencies.
  • Polish: The current implementation has some quirks. For example on my machine/account, every pilot I have ever made and synced is listed even though most have been deleted and I have seen reports of no pilots showing up.
  • Future: A more robust and blessed api for this may come in the future.
  • Performance: Removal of the aws code results in 1MB reduction in the js bundle, which is about 1/3 smaller than the current bundle.

Reasons we would want to keep this feature

  • Removal of a (mostly) working feature could upset users
  • Updating aws is difficult, but not intractable.
  • Extending this could possibly allow npc importing in the future.

@BoltsJ BoltsJ added C/C Issues relating to C/C import or integration. refactor Refactors of the code base for API changes, maintainability, etc... dependencies Pull requests that update a dependency file labels Jun 26, 2024
BoltsJ added 2 commits August 6, 2024 14:08
This alleviates the maintenance burden of trying  to keep up with aws
amplify updates. This resolves several security notices associated with
the amplify library and it's dependencies. This reduces the js bundle
size by about 1MB.
Removes the references to comp/con/ login and the dropdown which
depended on it from the pilot import tour.
@lazulit3
Copy link
Contributor

lazulit3 commented Aug 26, 2024

Your rationale / considerations all seem reasonable to me.

I am in favor of keeping the COMP/CON integration and find the pilot import features useful (though I should probably try setting up a pilot/mech directly in Foundry to get a feel for the hypothetical alternative workflow w/o import).

Maintenance: Keeping the aws dependencies updated represents a non-trivial amount of work.

I am willing to assist with the maintenance work around this.

#756 bumps aws-amplify to major version 5, but v6 is a more significant movement.

If getting aws-amplify on v6 is desired, Iet me know and I'm willing to do some discovery/exploration/testing around this (and implementation if I don't hit a wall).

Security: There are several current security notices for this repo due to aws-amplify and its dependencies.

I believe the current vulns are addressed by above PR (but I recognize that if more pop up with aws-amplify v5, resolving that would mean dealing with v5 -> v6 upgrade path).

Polish: The current implementation has some quirks. For example on my machine/account, every pilot I have ever made and synced is listed even though most have been deleted.

I tried to reproduce this a little without success. I may not be following the same steps as you, or I wonder if this could do with quirks around syncing deletions to cloud.

If you would like to open an issue with specific steps for reproducing this, I could take a look and try to determine whether this is an issue with COMP/CON or the system's integration. That does sound like a real nuisance.

Performance: Removal of the aws code results in 1MB reduction in the js bundle, which is about 1/3 smaller than the current bundle.

Out of curiosity, how do we measure this? Does "bundle" refer to the total size of the built contents of dist/ ?


Anyway, I don't think this proposal is unreasonable. Am willing to try to help with pain points around COMP/CON integration to the extent that I can. Feel free to point me at what you consider a priority.

@BoltsJ
Copy link
Collaborator Author

BoltsJ commented Aug 28, 2024

Amplify 5 has at least one vulnerable dependency (axios), so 6 is preferred to 5.
The issue with old data being retained doesn't happen to everyone, but it's still a pretty common issue. I went in a checked, and while a lot of old stuff was gone, creating then deleting a few pilots, including forcing immediate deletion, left me with several pilots in the dropdown that aren't part of my compcon data. This is especially annoying if you end up with duplicate entries. Even if it doesn't affect everyone, this is a significant ux problem.
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C Issues relating to C/C import or integration. dependencies Pull requests that update a dependency file refactor Refactors of the code base for API changes, maintainability, etc...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants