Skip to content

Add audit trail to the publish, yank and unyank transactions.#1700

Merged
bors merged 6 commits intorust-lang:masterfrom
markcatley:version-owner-actions
Dec 2, 2019
Merged

Add audit trail to the publish, yank and unyank transactions.#1700
bors merged 6 commits intorust-lang:masterfrom
markcatley:version-owner-actions

Conversation

@markcatley
Copy link
Copy Markdown
Contributor

@markcatley markcatley commented Mar 31, 2019

Contributes to Issue 1548
Based on the commits from PR 1604

markcatley
markcatley commented Mar 31, 2019
View reviewed changes
Comment thread src/models/action.rs Outdated
markcatley
markcatley commented Mar 31, 2019
View reviewed changes
Comment thread src/controllers/krate/publish.rs Outdated
markcatley
markcatley commented Mar 31, 2019
View reviewed changes
Comment thread src/controllers/version/yank.rs Outdated
@markcatley
Copy link
Copy Markdown
Contributor Author

markcatley commented Mar 31, 2019

What recommendations would you have on what should be unit tested?

markcatley
markcatley commented Mar 31, 2019
View reviewed changes
Comment thread migrations/2019-01-26-090348_create_version_owner_actions/up.sql
@markcatley markcatley force-pushed the version-owner-actions branch 2 times, most recently from 5da6778 to d03686b Compare March 31, 2019 23:37
markcatley
markcatley commented Mar 31, 2019
View reviewed changes
Comment thread src/models/action.rs Outdated
@bors
Copy link
Copy Markdown
Contributor

bors commented Apr 1, 2019

☔ The latest upstream changes (presumably #1677) made this pull request unmergeable. Please resolve the merge conflicts.

@markcatley markcatley force-pushed the version-owner-actions branch 3 times, most recently from bae2a11 to 5c8db82 Compare April 1, 2019 21:21
@markcatley markcatley force-pushed the version-owner-actions branch from 5c8db82 to 0fbb832 Compare May 3, 2019 00:23
This was referenced May 3, 2019
Merged
Merged
@markcatley markcatley force-pushed the version-owner-actions branch 2 times, most recently from 79ce0b6 to 15856bc Compare May 3, 2019 07:45
sgrif
sgrif previously requested changes May 13, 2019
View reviewed changes
Comment thread src/models/action.rs Outdated
Comment thread src/models/token.rs Outdated
Comment thread migrations/2019-01-26-090348_create_version_owner_actions/up.sql Outdated
Comment thread migrations/2019-01-26-090348_create_version_owner_actions/up.sql
Comment thread src/models/action.rs Outdated
@sgrif
Copy link
Copy Markdown
Contributor

sgrif commented May 13, 2019

I'd like to see some integration tests for this as well

@bors
Copy link
Copy Markdown
Contributor

bors commented Oct 5, 2019

☔ The latest upstream changes (presumably #1599) made this pull request unmergeable. Please resolve the merge conflicts.

@markcatley markcatley force-pushed the version-owner-actions branch 4 times, most recently from c4db368 to 6a0c3e6 Compare October 7, 2019 23:18
@markcatley
Copy link
Copy Markdown
Contributor Author

markcatley commented Oct 7, 2019

Sorry that I left this alone for so long. I noticed that @carols10cents merged PR 1604 that this was based on so I've updated this PR and made the changes suggested.

Some of the changes @sgrif made are related to code that's already been merged. I have not made these, and @carols10cents and @sgrif will have to decide which of these should be made.

@markcatley markcatley force-pushed the version-owner-actions branch 2 times, most recently from 653a122 to 2213b9c Compare October 8, 2019 21:20
carols10cents
carols10cents requested changes Nov 2, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

@markcatley I'm sorry we left this PR for so long! Thank you so much for sticking with it. I think we should definitely add some NOT NULL constraints as mentioned, and I would merge either way on renaming the fields in this PR or not.

@markcatley markcatley force-pushed the version-owner-actions branch 2 times, most recently from 464cb34 to 73c7238 Compare November 3, 2019 01:06
@markcatley
Copy link
Copy Markdown
Contributor Author

markcatley commented Nov 3, 2019

Hi @carols10cents @sgrif,

I think it's probably a good idea to rename those fields. Never an easier time than when the table is unused (I hope). I believe all the changes have now been made.

Kind regards,
Mark

@markcatley markcatley force-pushed the version-owner-actions branch 2 times, most recently from 34b9bf8 to 88d9ec0 Compare November 3, 2019 04:48
@markcatley markcatley force-pushed the version-owner-actions branch from 88d9ec0 to 8d35e31 Compare November 3, 2019 23:53
carols10cents
carols10cents requested changes Nov 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

This is getting really close! I just noticed a few small things on my latest read through-- please change the underscores in the parameter names and the comments in the tests where noted. Thank you!!

Comment thread src/models/token.rs
Comment thread src/models/action.rs Outdated
Comment thread migrations/2019-11-03-003946_tidy_up_version_owner_actions/up.sql
Comment thread src/tests/krate.rs Outdated
Comment thread src/tests/krate.rs Outdated
Comment thread src/tests/krate.rs Outdated
Comment thread src/tests/krate.rs Outdated
@carols10cents carols10cents self-assigned this Nov 15, 2019
@bors
Copy link
Copy Markdown
Contributor

bors commented Nov 25, 2019

☔ The latest upstream changes (presumably #1911) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Copy Markdown
Contributor

bors commented Nov 26, 2019

☔ The latest upstream changes (presumably #1927) made this pull request unmergeable. Please resolve the merge conflicts.

@carols10cents
Merge remote-tracking branch 'upstream/master' into version-owner-act…
3bf4557 
…ions
@carols10cents
Make parameter names consistent with conventions
a6430a0
@carols10cents
Clarify test description comments
3b1061f
@carols10cents
Remove some unnecessary assertions
e38744c 
The first audit action test covers testing that a crate starts without
any audit actions, and it's unlikely these assertions will ever fail.
@carols10cents
Copy link
Copy Markdown
Member

carols10cents commented Dec 2, 2019

I've resolved the merge conflicts and made the small outstanding fixes so that we can merge this in, which I'll do once CI passes :) Thanks!

@carols10cents carols10cents dismissed sgrif’s stale review December 2, 2019 16:37

all comments have been addressed

@carols10cents
Copy link
Copy Markdown
Member

carols10cents commented Dec 2, 2019

@bors r+

@bors
Copy link
Copy Markdown
Contributor

bors commented Dec 2, 2019

📌 Commit 4d7f8a8 has been approved by carols10cents

@bors
Copy link
Copy Markdown
Contributor

bors commented Dec 2, 2019

⌛ Testing commit 4d7f8a8 with merge f3e0511...

bors added a commit that referenced this pull request Dec 2, 2019
@bors
Auto merge of #1700 - markcatley:version-owner-actions, r=carols10cents
f3e0511 
Add audit trail to the publish, yank and unyank transactions.

Contributes to [Issue 1548](#1548)
Based on the commits from [PR 1604](#1604)
@bors
Copy link
Copy Markdown
Contributor

bors commented Dec 2, 2019

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing f3e0511 to master...

@bors bors merged commit 4d7f8a8 into rust-lang:master Dec 2, 2019
@bors bors mentioned this pull request Dec 2, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carols10cents carols10cents carols10cents approved these changes

+1 more reviewer

@sgrif sgrif sgrif left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@carols10cents carols10cents

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@markcatley @bors @sgrif @carols10cents