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

chore: un-nest CLIs, APIs, permissions and DB layer #10041

Merged
merged 7 commits into from
Oct 11, 2024

Conversation

ShreyaLnuHpe
Copy link
Contributor

@ShreyaLnuHpe ShreyaLnuHpe commented Oct 10, 2024

Ticket

DET-10462
DET-10465

Description

Test Plan

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@ShreyaLnuHpe ShreyaLnuHpe requested review from a team as code owners October 10, 2024 23:12
@ShreyaLnuHpe ShreyaLnuHpe requested review from MikhailKardash and salonig23 and removed request for a team October 10, 2024 23:12
@cla-bot cla-bot bot added the cla-signed label Oct 10, 2024
@ShreyaLnuHpe ShreyaLnuHpe marked this pull request as draft October 10, 2024 23:13
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 45.43081% with 209 lines in your changes missing coverage. Please review.

Project coverage is 54.56%. Comparing base (6d70ed4) to head (78a6a2d).

Files with missing lines Patch % Lines
harness/determined/cli/token.py 17.58% 75 Missing ⚠️
master/internal/token/authz_rbac.go 1.36% 72 Missing ⚠️
master/internal/token/authz_basic_impl.go 4.16% 23 Missing ⚠️
master/internal/api_token.go 82.11% 22 Missing ⚠️
master/internal/token/authz_permissive.go 10.00% 9 Missing ⚠️
master/internal/token/postgres_token.go 86.66% 8 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           shreya/createTable   #10041      +/-   ##
======================================================
- Coverage               59.16%   54.56%   -4.60%     
======================================================
  Files                     756     1266     +510     
  Lines                  105153   158044   +52891     
  Branches                 3632     3632              
======================================================
+ Hits                    62210    86238   +24028     
- Misses                  42809    71672   +28863     
  Partials                  134      134              
Flag Coverage Δ
backend 45.37% <53.79%> (+1.56%) ⬆️
harness 72.57% <19.35%> (-0.02%) ⬇️
web 54.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
harness/determined/cli/cli.py 45.13% <ø> (ø)
harness/determined/cli/user.py 53.57% <100.00%> (+15.14%) ⬆️
master/internal/api_user.go 70.21% <ø> (ø)
master/internal/user/authz_basic_impl.go 2.38% <ø> (ø)
master/internal/user/authz_permissive.go 2.38% <ø> (ø)
master/internal/user/authz_rbac.go 1.19% <ø> (ø)
master/internal/user/postgres_users.go 81.10% <ø> (ø)
master/internal/token/postgres_token.go 86.66% <86.66%> (ø)
master/internal/token/authz_permissive.go 10.00% <10.00%> (ø)
master/internal/api_token.go 82.11% <82.11%> (ø)
... and 3 more

... and 499 files with indirect coverage changes

@ShreyaLnuHpe ShreyaLnuHpe marked this pull request as ready for review October 10, 2024 23:44
Copy link

cla-bot bot commented Oct 11, 2024

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @determined-ci, @jgongd on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

@cla-bot cla-bot bot removed the cla-signed label Oct 11, 2024
@determined-ci determined-ci requested a review from a team October 11, 2024 06:06
@determined-ci
Copy link
Collaborator

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

@determined-ci determined-ci added the documentation Improvements or additions to documentation label Oct 11, 2024
Copy link

cla-bot bot commented Oct 11, 2024

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @determined-ci, @jgongd on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

@determined-ci determined-ci removed the documentation Improvements or additions to documentation label Oct 11, 2024
Copy link
Contributor

@corban-beaird corban-beaird left a comment

Choose a reason for hiding this comment

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

Looks solid! Thanks for getting this knocked out so quickly! Maybe just do a quick rebase against the feature branch to clean up the extra migrations commits and this should be good to go!

# fmt: off

args_description = [
cli.Cmd("to|ken", None, "manage access tokens", [
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that you added a shortened version of token here, adds a clearer polish to this new set of subcommands!

@determined-ai determined-ai deleted a comment from determined-ci Oct 11, 2024
@ShreyaLnuHpe ShreyaLnuHpe merged commit 5d887c4 into shreya/createTable Oct 11, 2024
111 of 115 checks passed
@ShreyaLnuHpe ShreyaLnuHpe deleted the shreya/unnesting branch October 11, 2024 16:23
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