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

feat(byods): create the main BYODS class implementation #3824

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

bhabalan
Copy link
Contributor

@bhabalan bhabalan commented Sep 11, 2024

COMPLETES #SPARK-549037

This pull request addresses

Creation of the main class for the BYoDS sdk

by making the following changes

  • Mechanism to store tokens for multiple orgIds
  • Expose an HTTP client that is bound with orgId and access token
  • Fetch the JWKS in the constructor

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios where tested

I certified that

  • I have read and followed contributing guidelines

  • I discussed changes with code owners prior to submitting this pull request

  • I have not skipped any automated checks

  • All existing and new tests passed

  • I have updated the documentation accordingly


Make sure to have followed the contributing guidelines before submitting.

@bhabalan bhabalan self-assigned this Sep 11, 2024
@bhabalan bhabalan requested a review from a team as a code owner September 11, 2024 03:46
@bhabalan bhabalan changed the title feat(byods): create the foundation class implementation feat(byods): create the main BYoDS class implementation Sep 11, 2024
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
@Kesari3008
Copy link
Contributor

Please add the validated label for the checks to run

- Refactored constants into a separate module for better manageability
- Moved HTTP calls to BaseClient
- Introduced DataSourceClient for interaction with the dataSource API
- Created TokenManager to centralize token handling and refresh logic
@bhabalan bhabalan added the validated If the pull request is validated for automation. label Sep 12, 2024
Copy link
Contributor

@sreenara sreenara left a comment

Choose a reason for hiding this comment

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

Please move each of the clients into a separate folder with its own types file so each folder has its own type. Common types can stay at the root.
Additionally, we should adhere to the structure used by plugin-meetings and others for the tests as the tooling looks for a directory structure similar to them.
For e.g. packages/byods/test/unit/spec/byods/index.js & packages/byods/test/unit/spec/tokenManager/index.js

packages/byods/src/BYoDS.test.ts Outdated Show resolved Hide resolved
packages/byods/package.json Outdated Show resolved Hide resolved
packages/byods/package.json Outdated Show resolved Hide resolved
packages/byods/package.json Outdated Show resolved Hide resolved
packages/byods/src/index.ts Outdated Show resolved Hide resolved
packages/byods/src/types.ts Outdated Show resolved Hide resolved
packages/byods/src/types.ts Show resolved Hide resolved
packages/byods/src/constants.ts Outdated Show resolved Hide resolved
packages/byods/src/types.ts Outdated Show resolved Hide resolved
packages/byods/src/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rarajes2 rarajes2 left a comment

Choose a reason for hiding this comment

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

First round of review is done. May need another review.

@bhabalan - Please rename all the files to kebab-case, we follow the PascalCase convention for react applications.

packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BYODS.ts Outdated Show resolved Hide resolved
packages/byods/src/BaseClient.ts Outdated Show resolved Hide resolved
packages/byods/src/BaseClient.ts Outdated Show resolved Hide resolved
packages/byods/src/TokenManager.ts Outdated Show resolved Hide resolved
packages/byods/src/TokenManager.ts Outdated Show resolved Hide resolved
packages/byods/src/TokenManager.ts Outdated Show resolved Hide resolved
packages/byods/src/types.ts Outdated Show resolved Hide resolved
packages/byods/src/types.ts Outdated Show resolved Hide resolved
@bhabalan bhabalan changed the title feat(byods): create the main BYoDS class implementation feat(byods): create the main BYODS class implementation Sep 17, 2024
@bhabalan bhabalan force-pushed the feat/byods-base-class branch from 56e4a36 to f68f236 Compare September 18, 2024 03:16
@sreenara sreenara dismissed Kesari3008’s stale review September 18, 2024 05:27

Priya is on leave

Copy link
Contributor

@sreenara sreenara left a comment

Choose a reason for hiding this comment

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

First round of review is done. May need another review.

@bhabalan - Please rename all the files to kebab-case, we follow the PascalCase convention for react applications.

@rarajes2 are there other instances of kebab-case?

/**
* The audience for the data source request.
*/
audience: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Question. Is audience just a string for the developer to use for context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It could be anything. Sort of like a note for the developer to know what the dataSource is for.

@rarajes2
Copy link
Contributor

First round of review is done. May need another review.

@bhabalan - Please rename all the files to kebab-case, we follow the PascalCase convention for react applications.

@rarajes2 are there other instances of kebab-case?

kebab-case is the general convention that we follow. For react PascalCase was introduced.

@bhabalan bhabalan merged commit 8f30732 into webex:feat/byods Sep 18, 2024
10 checks passed
bhabalan added a commit to bhabalan/webex-js-sdk that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated for automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants