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

[Enhancement] [ALS-1792] Update creds and websocket implementation #235

Conversation

wadhawh
Copy link
Collaborator

@wadhawh wadhawh commented Aug 14, 2024

This PR has the following changes;

  • Removes dependency of guest and auth creds from aws-amplify
  • Removes dependency of websocket from aws-amplify
  • Updates E2E workflow for simplicity
  • Updates naming convention for services

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

);
};
const AppWrapper: FC<AppWrapperProps> = ({ children }) => (
<ThemeProvider theme={appTheme} nonce="dAnIsRazNonCe">
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the nonce supposed to be randomly generated on every request?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be, I'll look into this. This was actually worked on by another developer at the start of this project. Even if we remove this, we will be good. I'll give that a test and let you know.

}
timeout = setTimeout(() => {
refreshCredentials();
}, differenceInMilliseconds(new Date(credentials.expiration || 0), new Date()) - 300);
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous logic didn't have the "- 300", why is that needed now?

Copy link
Collaborator

@its-aazizi its-aazizi Aug 21, 2024

Choose a reason for hiding this comment

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

Credentials are refreshed 5 mins before expiration. I see that the 300 is incorrect, I'll update this ASAP.

@its-aazizi
Copy link
Collaborator

@mbalfour-amzn raising another PR, since the PR I raised from forked repo shows conflicts

@wadhawh wadhawh added the stale label Aug 21, 2024
@wadhawh wadhawh marked this pull request as draft August 21, 2024 10:40
@its-aazizi
Copy link
Collaborator

@mbalfour-amzn this PR has been marked as stale and moved in draft. You can find the updated PR here that resolves the comments in this PR. This PR can be closed and branch deleted.

@mbalfour-amzn mbalfour-amzn deleted the enhancement_ALS-1792_update-creds-and-websocket-implementation branch August 21, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants