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

owners remove/ add response does not fit documentation #12056

Closed
2ndDerivative opened this issue Apr 28, 2023 · 9 comments
Closed

owners remove/ add response does not fit documentation #12056

2ndDerivative opened this issue Apr 28, 2023 · 9 comments
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-interacts-with-crates.io Area: interaction with registries C-bug Category: bug E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@2ndDerivative
Copy link

Problem

The Registry Web API documentation says the Owner --remove command needs a JSON object

{
    "ok":true
}

even though it needs the same OwnerResponse object as the add command, which also has a msg field.
This causes registries that fit the documentation API to error out the user for an incomplete JSON when removing a user.

Steps

Respond to the Owner remove API request with a JSON object without the "msg" field.
Cargo will error out with

error: failed to remove owners from crate `[crate-name]` on registry at [endpoint]

Caused by:
  missing field `msg` at line 1 column 11

Possible Solution(s)

Either the OwnerResponse object should probably be split into AddResponse and RemoveResponse to fit the documentation, or the msg field on the remove method should be used, or the msg field should be made optional.

Notes

No response

Version

No response

@2ndDerivative 2ndDerivative added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Apr 28, 2023
@2ndDerivative 2ndDerivative changed the title owners remove/ add response is not equivalent owners remove/ add response does not fit documentation Apr 28, 2023
@weihanglo
Copy link
Member

Just looked into crates.io implementation, it always responds a JSON of { ok, msg }.

Given Cargo already fails when missing that field, I would recommend adding the msg field to the documentation. It shouldn't be a breaking change to registry implementors.

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review A-documenting-cargo-itself Area: Cargo's documentation A-interacts-with-crates.io Area: interaction with registries E-easy Experience: Easy and removed S-triage Status: This issue is waiting on initial triage. labels Apr 28, 2023
@weihanglo
Copy link
Member

Place that needs a tweak:

```javascript
{
// Indicates the remove succeeded, always true.
"ok": true
}
```

@2ndDerivative
Copy link
Author

I'd also suggest putting in the message display into cargo, I can do that later

@Bhardwaj-Himanshu
Copy link
Contributor

Hi everyone, from my understanding
this-

{ 
    // Indicates the remove succeeded, always true. 
    "ok": true 
} 

should be replaced to something like-

{ 
   // some kind of message here
   "ok": true 
} 

But being unsure of how AddResponse and RemoveResponse work, I am unable to sense what changes to make and how.
Are these 2 seperate functions which need to be added as-

Addresponse { 
    // something_1
    "ok": true 
} 

and

RemoveResponse{ 
    // something_1
    "ok": true 
}

Or Add response and Remove response are just messages that need to be displayed whenever user interacts with this?

Also, let me know if someone is upto some ideas and how he/she is working on this.
Thanks

@2ndDerivative
Copy link
Author

2ndDerivative commented May 1, 2023

No the Json object response to cargo owner --remove needs to be replaced by

{
    "message": "Some message here",
    "ok": true
}

because the reponse cargo expects is the same as the response to cargo owner --add.

@Bhardwaj-Himanshu
Copy link
Contributor

Bhardwaj-Himanshu commented May 1, 2023

So the message which needs to go in there should be like

{
     "message": "task performed successfully",

     #not writing owner removed or owner added       here, as mentioned by you, the object responds similarly for both the cases.
      "ok": "true"

}

This message was written via phone, so pardon me for the improper syntax here;)

@2ndDerivative
Copy link
Author

The message field in the reponse to owner --remove is not used in any way, but cargo expects it to just exist. So so far there can be anything there.

what we need to change is the documentation to recommend putting a message there to API implementors and warning that it's needed but not used (yet).

bors added a commit that referenced this issue May 1, 2023
docs(registry): Further specify owner-remove response (#12056)

### What does this PR try to resolve?

This fixes a documentation issue as mentioned in #12056.

### How should we test and review this PR?

This is just a markdown file change, so it does not break anything.

### Additional information

Let me know if you have any changes or suggestions or how to write the tests for markdown files as well!

Thanks
bors added a commit that referenced this issue May 2, 2023
docs(registry): Further specify owner-remove response (#12056)

### What does this PR try to resolve?

This fixes a documentation issue as mentioned in #12056.

### How should we test and review this PR?

This is just a markdown file change, so it does not break anything.

### Additional information

Let me know if you have any changes or suggestions or how to write the tests for markdown files as well!

Thanks
bors added a commit to rust-lang-ci/rust that referenced this issue May 3, 2023
Update cargo

16 commits in 9e586fbd8b931494067144623b76c37d213b1ab6..ac84010322a31f4a581dafe26258aa4ac8dea9cd
2023-04-25 22:09:11 +0000 to 2023-05-02 13:41:16 +0000
- docs(registry): Further specify owner-remove response (rust-lang/cargo#12056) (rust-lang/cargo#12068)
- Remove repeated definite articles (rust-lang/cargo#12067)
- Document that adding `#[non_exhaustive]` on existing items is breaking. (rust-lang/cargo#10877)
- docs(commands): add missed preposition (rust-lang/cargo#12073)
- Fix warning with unused mut (rust-lang/cargo#12065)
- chore: move build-man workflow away from shell (rust-lang/cargo#12048)
- feat: Add `-Zmsrv-policy` feature flag (rust-lang/cargo#12043)
- chore: new xtask to check stale paths in autolabel defintions (rust-lang/cargo#12051)
- cargo-tree: Handle -e no-proc-macro when building the graph (rust-lang/cargo#12044)
- chore: update trigger_files in autolabel (rust-lang/cargo#12052)
- fix broken markdown in docs (rust-lang/cargo#12049)
- home: fix & enhance documentation (rust-lang/cargo#12047)
- chore: Mark unpublished crates as such (rust-lang/cargo#12045)
- Include rust-version in publish request (rust-lang/cargo#12041)
- chore(xtask): Add `cargo xtask unpublished` (rust-lang/cargo#12039)
- docs(ref): Specify 'rust_version' in Index format (rust-lang/cargo#12040)

r? `@ghost`
RalfJung pushed a commit to RalfJung/miri that referenced this issue May 3, 2023
Update cargo

16 commits in 9e586fbd8b931494067144623b76c37d213b1ab6..ac84010322a31f4a581dafe26258aa4ac8dea9cd
2023-04-25 22:09:11 +0000 to 2023-05-02 13:41:16 +0000
- docs(registry): Further specify owner-remove response (rust-lang/cargo#12056) (rust-lang/cargo#12068)
- Remove repeated definite articles (rust-lang/cargo#12067)
- Document that adding `#[non_exhaustive]` on existing items is breaking. (rust-lang/cargo#10877)
- docs(commands): add missed preposition (rust-lang/cargo#12073)
- Fix warning with unused mut (rust-lang/cargo#12065)
- chore: move build-man workflow away from shell (rust-lang/cargo#12048)
- feat: Add `-Zmsrv-policy` feature flag (rust-lang/cargo#12043)
- chore: new xtask to check stale paths in autolabel defintions (rust-lang/cargo#12051)
- cargo-tree: Handle -e no-proc-macro when building the graph (rust-lang/cargo#12044)
- chore: update trigger_files in autolabel (rust-lang/cargo#12052)
- fix broken markdown in docs (rust-lang/cargo#12049)
- home: fix & enhance documentation (rust-lang/cargo#12047)
- chore: Mark unpublished crates as such (rust-lang/cargo#12045)
- Include rust-version in publish request (rust-lang/cargo#12041)
- chore(xtask): Add `cargo xtask unpublished` (rust-lang/cargo#12039)
- docs(ref): Specify 'rust_version' in Index format (rust-lang/cargo#12040)

r? `@ghost`
@Rustin170506
Copy link
Member

@weihanglo I guess we can close this issue now.

@weihanglo
Copy link
Member

Surely. Thanks!

Fixed by #12068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-interacts-with-crates.io Area: interaction with registries C-bug Category: bug E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

4 participants