Skip to content

Conversation

@nora-shap
Copy link
Member

Description

Tokenless v2 has requirements for branch format, True Tokenless is allowed on any branch.

There are a few places in code (across several projects) where a request without a token is rejected if it doesn't have the fork format (branch contains :). I need to remove those blocks and allow those requests through. There are auth methods for True Tokenless in shelter and api, the validity of the tokenless uploads will be checked there.

true tokenless auth check for BA on api: codecov/codecov-api#882

I couldn't find instructions for local dev, so I will need help with lint and tests, made my best guess for both 🙃

codecov/engineering-team#2877

@codecov
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.08%. Comparing base (b17f870) to head (c1f8e2a).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
Components Coverage Δ
Plugin core 97.06% <100.00%> (-0.01%) ⬇️
Rollup plugin 10.81% <ø> (ø)
Vite plugin 11.02% <ø> (ø)
Webpack plugin 49.88% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-staging
Copy link

codecov-staging bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Components Coverage Δ
Plugin core 97.06% <100.00%> (-0.01%) ⬇️
Rollup plugin 10.81% <ø> (ø)
Vite plugin 11.02% <ø> (ø)
Webpack plugin 49.88% <ø> (ø)

📢 Thoughts on this report? Let us know!

@codecov
Copy link

codecov bot commented Nov 13, 2024

Bundle Report

Changes will decrease total bundle size by 3.54kB (-0.06%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@codecov/vite-plugin-esm 1.24kB 1.08kB (-46.64%) ⬇️
@codecov/remix-vite-plugin-esm 957 bytes 133 bytes (-12.2%) ⬇️
@codecov/bundler-plugin-core-cjs 58.98kB 193 bytes (-0.33%) ⬇️
@codecov/bundler-plugin-core-esm 53.11kB 159 bytes (-0.3%) ⬇️
@codecov/rollup-plugin-esm 1.3kB 1.02kB (-43.95%) ⬇️
@codecov/sveltekit-plugin-esm 891 bytes 198 bytes (-18.18%) ⬇️
@codecov/example-sveltekit-app-server-esm 974.29kB 1 bytes (0.0%) ⬆️
@codecov/nextjs-webpack-plugin-esm 1.11kB 753 bytes (-40.33%) ⬇️

@codecov-staging
Copy link

codecov-staging bot commented Nov 13, 2024

Bundle Report

Changes will decrease total bundle size by 764 bytes (-0.03%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@codecov/bundler-plugin-core-cjs 58.98kB 193 bytes (-0.33%) ⬇️
@codecov/bundler-plugin-core-esm 53.11kB 159 bytes (-0.3%) ⬇️
@codecov/bundle-analyzer-esm 3.45kB 347 bytes (11.17%) ⬆️
@codecov/nextjs-webpack-plugin-esm 1.11kB 753 bytes (-40.33%) ⬇️
@codecov/webpack-plugin-esm 3.36kB 6 bytes (-0.18%) ⬇️

Copy link
Contributor

@suejung-sentry suejung-sentry left a comment

Choose a reason for hiding this comment

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

For example/tokenless - what setup do we do on the repo/github actions/other to make that go through successfully? Said another way - is there somewhere I can read to learn more on how to get "true tokenless" configured? Mostly to make sure our tests in here will continue to cover the scenario

codecovVitePlugin({
enableBundleAnalysis: true,
bundleName: "@codecov/example-tokenless-app",
uploadToken: process.env.TOKENLESS_UPLOAD_TOKEN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we still supporting the "tokenlessForForks"? If so, we should add a separate example for that scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we are keeping tokenlessForForks live. good call - I agree there should be an example/test to show that tokenlessForForks are still valid

Choose a reason for hiding this comment

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

Would platform side of things complain if we set the branch to codecov:<branch-name> for the example?

Choose a reason for hiding this comment

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

or does it have to be a username?

Copy link
Member Author

Choose a reason for hiding this comment

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

might be hard to fudge the :

to get the branch:
first - for BA, shelter gets the commit_id from the commit field in the body of the request. The commit and repo have to exist in firestore - I'm thinking they will. If they exist, shelter uses the branch from the commit.

if shelter can't get the commit (if you found a way to break the above method), it will use the branch field in the body of the request.

and then we have an additional check just for BA uploads, that the branch (most likely from the commit) has to match the branch in the body of the request.

This might explain why the tokenless example doesn't work on shelter

@nora-shap
Copy link
Member Author

I learned a lot from @suejung-sentry about examples and how they work.

I didn't realize they were using real data from the pr. This means an example for True Tokenless will never succeed to upload, because the Codecov Org will never allow tokenless uploads (it's determined by a field on the OwnerOrg). So if you want an example of a passing True Tokenless upload, you need to run it on a different OwnerOrg, which becomes more of an integration test than an example.

We also learned that the current tokenless example has been failing since the switch to shelter, so this example will need tweaks if you want it to pass, but that doesn't have to do with the changes in this pr, so I'm going to merge this pr.

After this pr is merged, BA will work for True Tokenless, and will continue to not work for tokenless v2

Copy link
Contributor

@suejung-sentry suejung-sentry left a comment

Choose a reason for hiding this comment

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

We can add integration tests for the tokenless v3 (true tokenless) scenario and also a separate example org demonstrating true tokenless with a separate ticket

@nicholas-codecov
Copy link

Yea if we're unable to provide a true tokenless example that also uploads data, I'm alright still having something in there, that people can look at.

@nicholas-codecov nicholas-codecov changed the title do not filter out branches for tokenless uploads feat: Enable true tokenless for bundler plugins Nov 15, 2024
@nicholas-codecov nicholas-codecov merged commit 2db57cc into main Nov 15, 2024
66 checks passed
@nicholas-codecov nicholas-codecov deleted the nora/2877 branch November 15, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants