Skip to content

/crater

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

Auto-increment default names for PRs #334

Merged
merged 8 commits into from Oct 11, 2018

Conversation

Reviewers

@pietroalbini pietroalbini

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@devonhollowood @pietroalbini @bors
@devonhollowood
Copy link
Contributor

devonhollowood commented Oct 5, 2018

Fix #306

I thought that the easiest way to go about this was to adjust default_experiment_name(), and to just have it automatically try new names in case of a clash until it finds one that works. This makes the assumption that there won't be too too many experiments for a given PR, since experiment N will make N calls to the database, which is only really okay if N is small. Let me know if you'd like me to do something smarter here, or to adjust a different area of the code instead.

devonhollowood added some commits Oct 5, 2018

@devonhollowood
Auto-increment default names for PRs
Loading status checks…
59dc446
@devonhollowood
Fix formatting
Loading status checks…
8b8ce4f
@pietroalbini

This comment has been minimized.

Sign in to view
Copy link
Member

pietroalbini commented Oct 5, 2018

Thanks for the PR!

Unfortunately there is a bug with your implementation: you always generate a new name for the experiment, but the new name should only be generated for the run command. Other commands should only return the latest one.

As for the n+1 queries, it shouldn't really be a problem (I don't expect to have more than 5 runs for a single PR). If you still want to avoid that you could select the count of experiments pointing to the same PR (checking the API URL in the database).

Example of the bug

schermata del 2018-10-05 21 12 54
@devonhollowood

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

devonhollowood commented Oct 5, 2018

Ah, that makes sense. I'll take another shot at this tonight then.
@devonhollowood

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

devonhollowood commented Oct 7, 2018

Okay, I think it should work now. If not, I'll try to catch you on Discord to discuss some more.

Let me know if you want me to squash these commits or anything.
@pietroalbini

This comment has been minimized.

Sign in to view
Copy link
Member

pietroalbini commented Oct 8, 2018

There are still a few quirks in the implementation: if a second run is created and then edited, the edit command uses the first run's name.

Example of the bug

screenshot_2018-10-08_21-54-21

The way I'd like to see this implemented is the following:

  • If the command creates a new experiment (run) always store the name in the database
    • If the user provides a custom name store that
    • Otherwise just generate a new one with the logic you wrote
  • If the command changes an existing experiment:
    • If the user provided a custom name use that and store it in the database
    • Otherwise look for a saved name in the database
    • If no saved name is present don't generate a new name, instead return an error

If you have any doubt just ping me on Discord!

devonhollowood added some commits Oct 8, 2018

@devonhollowood
Store auto-incremented names 
Also, don't auto-increment user-supplied names
Loading status checks…
9706946
@devonhollowood
Fix formatting
Loading status checks…
c33ddd3
@pietroalbini

pietroalbini requested changes Oct 9, 2018
View changes

src/server/routes/webhooks/commands.rs Outdated
if name_supplied {
name
} else {
let new_name = auto_increment_experiment_name(&data.db, &name)?;

This comment has been minimized.

Sign in to view
Copy link
@pietroalbini

pietroalbini Oct 9, 2018

Member

By passing as argument the previously stored name this is generating weird results, like last-name-1-1-1 (like pr-123-1-1-1-1).
src/server/routes/webhooks/commands.rs Outdated
@@ -15,7 +15,17 @@ pub fn ping(data: &Data, issue: &Issue) -> Result<()> {
}

pub fn run(host: &str, data: &Data, issue: &Issue, args: RunArgs) -> Result<()> {
let name = get_name(&data.db, issue, args.name)?;
let name = {
let name_supplied = args.name.is_some();

This comment has been minimized.

Sign in to view
Copy link
@pietroalbini

pietroalbini Oct 9, 2018

Member

This can be factored out as a standalone function, so it's easily testable.
@devonhollowood
Improve tests and fix behavior 
See discussion at #334
Loading status checks…
7032f85
@devonhollowood

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

devonhollowood commented Oct 10, 2018

I think that commit addresses the issues you brought up and behaves as specified; let me know if I missed something.
@pietroalbini

pietroalbini requested changes Oct 10, 2018
View changes

Copy link
Member

pietroalbini left a comment

This works great now, thanks! I left a small comment, and then this can be merged.
src/server/routes/webhooks/commands.rs Outdated
name = format!("pr-{}-{}", issue.number, idx);
idx = idx
.checked_add(1)
.expect("too many similarly-named pull requests");

This comment has been minimized.

Sign in to view
Copy link
@pietroalbini

pietroalbini Oct 10, 2018

Member

Can you avoid panicking here? I'd prefer for the server not to crash if someone creates way too many experiments.

This comment has been minimized.

Sign in to view
Copy link
@devonhollowood

devonhollowood Oct 10, 2018

Author Contributor

Done!
@pietroalbini

pietroalbini approved these changes Oct 11, 2018
View changes

@pietroalbini

This comment has been minimized.

Sign in to view
Copy link
Member

pietroalbini commented Oct 11, 2018

Thanks!

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Collaborator

bors commented Oct 11, 2018

📌 Commit b763389 has been approved by pietroalbini
@bors

This comment has been minimized.

Sign in to view
Copy link
Collaborator

bors commented Oct 11, 2018

⌛️ Testing commit b763389 with merge d2e54c1...

bors added a commit that referenced this pull request Oct 11, 2018

@bors
Auto merge of #334 - devonhollowood:auto-increment-default-name, r=pi… 
…etroalbini

Auto-increment default names for PRs

Fix #306

I thought that the easiest way to go about this was to adjust `default_experiment_name()`, and to just have it automatically try new names in case of a clash until it finds one that works. This makes the assumption that there won't be too too many experiments for a given PR, since experiment `N` will make N calls to the database, which is only really okay if N is small. Let me know if you'd like me to do something smarter here, or to adjust a different area of the code instead.
Loading status checks…
d2e54c1
@bors

This comment has been minimized.

Sign in to view
Copy link
Collaborator

bors commented Oct 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pietroalbini
Pushing d2e54c1 to master...

@bors bors merged commit b763389 into rust-lang-nursery:master Oct 11, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pietroalbini

This comment has been minimized.

Sign in to view
Copy link
Member

pietroalbini commented Oct 11, 2018

Changes deployed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.