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

fix(owner): Fix the currently broken yarn owner command #5593

Merged
merged 2 commits into from
Apr 13, 2018

Conversation

rubycut
Copy link
Contributor

@rubycut rubycut commented Mar 28, 2018

Summary

Test plan

Yarn now correctly displays the list of package owners. Also added automated tests.

const pkg = await config.registries.npm.request(name);
reporter.step(1, 1, reporter.lang('ownerGetting', name));
const pkg = await config.registries.npm.request(name, {
headers: {Accept: 'application/json'},
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to specify the headers? Isn't config.registries.npm doing it for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried without headers and it didn't work, and such json headers appear in other commands also.

Copy link
Member

Choose a reason for hiding this comment

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

@arcanis - maybe we can add this by default in a separate diff if it is not done already?

@arcanis arcanis self-assigned this Apr 4, 2018
@BYK
Copy link
Member

BYK commented Apr 11, 2018

@rubycut can you merge from master and see if that fixes the broken builds?

@rubycut
Copy link
Contributor Author

rubycut commented Apr 13, 2018

@BYK , I rebased to master, nothing changed.

macos checks always fail (free plan?), appveyor also fails, which doesn't seem to be connect with my pull request, appveyor error message:

Build started
git clone -q https://github.com/yarnpkg/yarn.git C:\projects\yarn
git fetch -q origin +refs/pull/5593/merge:
git checkout -qf FETCH_HEAD
An error occurred while sending the request.

@BYK
Copy link
Member

BYK commented Apr 13, 2018

macos checks always fail (free plan?), appveyor also fails, which doesn't seem to be connect with my pull request, appveyor error message:

This doesn't happen on master or on other pull requests. Something is up with how this is set up. I'll see what's wrong. For some reason both services are trying to use your account instead of Yarn's account.

@BYK BYK changed the title fix owner command and start working on tests fix(owner): Fix the currently broken yarn owner command Apr 13, 2018
@BYK BYK merged commit 8a7ee77 into yarnpkg:master Apr 13, 2018
@BYK
Copy link
Member

BYK commented Apr 13, 2018

Thanks a lot!

BYK added a commit that referenced this pull request Apr 13, 2018
**Summary**

Follow up to #5593. Looks like we need to send `Accept: application/json` header with most of the
registry requests so this patch adds it by default and removes the ad-hoc headers from info and
owner commands.

**Test plan**

Existing tests should pass.
@rubycut
Copy link
Contributor Author

rubycut commented Apr 14, 2018

@BYK when I started development, I opened my own circle-ci account, because I was working on enterprise tests, maybe that was the issue.

If I fork your repo, can I automatically run tests on circle-ci during development in forked repo without opening account there? I doubt that.

So, what is is the proper workflow?

@BYK
Copy link
Member

BYK commented Apr 15, 2018

@rubycut - ah, that probably explains it. You can't use Yarn's CircleCI account unless you submit a PR to it. Once a PR is opened, CircleCI should run that specific PR in Yarn's account. I'll let them know of this issue.

Thanks for the help!

BYK added a commit that referenced this pull request Apr 15, 2018
…from npm (#5667)

**Summary**

Follow up to #5593. Looks like we use an npm-specific request header to reduce the bandwidth. That said, for some commands, we need the full responses, which was achieved by setting the `Accept: application/json` header manually. This patch introduces a more descriptive request option, `unfiltered` to standardize this usage.

See https://github.com/npm/npm-registry-client#requests for more info.

**Test plan**

Existing tests should pass.
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.

3 participants