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.

Sign up for GitHub

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

Jump to bottom

API Token and Storage Token Update #202

Merged
merged 36 commits into from
Jul 19, 2023
Merged

API Token and Storage Token Update #202

merged 36 commits into from
Jul 19, 2023

Conversation

nh916
Copy link
Contributor

@nh916 nh916 commented Jul 14, 2023 •
edited
Loading

Description

frontend now allows users to grab http token and cloud storage from security page. This update is changed to reflect working with that.

Changes

  • renamed token to api_token to differentiate between different kinds of tokens
  • putting Bearer in front of the token within the code and user can just pass in a raw JWT and we prepare it for them
  • updated reading from config.json and environment variables accordingly
    • updated example script docs as well to use the new format
  • added storage_token to interact with AWS S3 in cognito mode
  • changed docs/exampls/synthesis.md api initialization to pass the CI
    • if we want to have them blank and read env vars automatically, then we'll have to alter our tests.yaml workflow that I would rather not do in this PR

Tests

  • the file upload and download test worked well
  • interacting with the API worked well
  • tests that need to be skipped are no longer commented out, but are using pytest skip
  • wrote test for initializing api with None

  • not every different way to initialize the SDK has a test written so there is a chance that if the user mixes the configurations there could be issues

  • more testing is needed because there are several options and something could clash or something could break with one of the options

Known Issues

  • lack of testing covering all scenarios for the initialization
  • having multiple options for initialization can lead to bugs that are harder to track down and fix
  • My name is on the list of contributors (CONTRIBUTORS.md) in the pull request source branch.
  • I have updated the documentation to reflect my changes.

@nh916
frontend implemented feature to get token from UI
efcdb21 
updated code to reflect using of the new token
@trunk-io
Copy link

trunk-io bot commented Jul 14, 2023

Merging to develop in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

nh916 added 18 commits July 18, 2023 11:55
@nh916 nh916 changed the title Frontend implemented UI API Token HTTP API Token and Storage Token Jul 18, 2023
@nh916 nh916 changed the title HTTP API Token and Storage Token HTTP API Token and Storage Token Update Jul 18, 2023
@nh916
wrote test_api_cript_env_vars for test_api.py
b41cd07
@nh916
verified environment variables host and token with test
a340b67
@nh916
skipping tests correctly
4705d0c 
* skipping tests with pytest skip tests and giving a good reason for each one

* removing unneeded `cript_api` argument for tests that don't need it

* uncommented the tests and the imports at the top
     * optimized imports as well
@nh916
fixed broken test test_api_with_invalid_host
a98d147
@nh916
updated
add6780
@nh916
formatted with black
2d855b2
@nh916
renamed http_token to api_token
ec0f6ef
@nh916 nh916 changed the title HTTP API Token and Storage Token Update API Token and Storage Token Update Jul 18, 2023
@nh916 nh916 marked this pull request as ready for review July 18, 2023 23:57
@nh916 nh916 requested a review from InnocentBug July 19, 2023 00:14
InnocentBug
InnocentBug requested changes Jul 19, 2023
View reviewed changes
Copy link
Collaborator

@InnocentBug InnocentBug left a comment

Choose a reason for hiding this comment

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

The tests aren’t passing yet.

It is a working design, I think but not great.

How does the front end get the storage token?
Is there an end point where it is requested from?

ideally we would not have the user provide the storage token separately.

Instead we request that token like the front end. And validate that request with the api token as before.

@nh916
undoing changes to test_integration.py
f6d8916 
the skip did not work on it because it is only a function and the pytest skip decorator needs to go on all the actual tests for it to skip correctly
@nh916
formatted with black
3f05974
@nh916
Copy link
Contributor Author

nh916 commented Jul 19, 2023 •
edited
Loading

The tests aren’t passing yet.

It is a working design, I think but not great.

How does the front end get the storage token? Is there an end point where it is requested from?

ideally we would not have the user provide the storage token separately.

Instead we request that token like the front end. And validate that request with the api token as before.

This is currently the frontend
image

I would use one token ideally but this is the resource that I have from the API and frontend, since we have higher priority things to work on, I am working with what I have and trying to get it done. We can always improve it after a working version

@nh916 nh916 requested a review from InnocentBug July 19, 2023 17:25
@nh916 nh916 merged commit 20de1bf into develop Jul 19, 2023
14 checks passed
@nh916 nh916 deleted the update_api_token branch July 19, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InnocentBug InnocentBug InnocentBug approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nh916 @InnocentBug