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

Add glacier command for adding ICE snippets easily #234

Closed
wants to merge 1 commit into from

Conversation

hellow554
Copy link

This is a first implementation of the glacier command for triagebot.

The "tracking issue" is rust-lang/glacier#231

I want a small feedback on the parser itself and its syntax.
Is there a command am I missing? How is the usability?

One thing I don't like is that I have to quote the URL, but I don't see a easy way around. Any thoughts on it?

@JohnTitor
Copy link
Member

What do you expect from glacier remove?

@Mark-Simulacrum
Copy link
Member

It is not immediately apparent to me what the CodeSource::Post is intended to be -- I think we would not want to try to parse a codeblock from the input comment. I would prefer that we stick to just parsing the URL as you've suggested. One other possibility is accepting a GitHub gist URL. I'm not sure what our requirements on glacier's side are.

@hellow554
Copy link
Author

(I should explain my thought a little bit more next time 😅)

The intention for glacier remove was that an ICE can be removed from the repository (e.g. somebody added one, but a maintainer decided, that it already exists as a dup or something else (malicious code).

CodeSource::Post was thought as a quicker way to add an ICE than using the playground. If ony one codeblock (```rust code... ```) is available that one will be taken.
I thought about using gist as well, but multiple files can be added to one gist, which I don't know yet how to handle. If we use the playground, a gist will be created in the background as well, but only one file, so that would be okay.

@Mark-Simulacrum
Copy link
Member

Let's stick to just the playpen for now.

I would expect this command to be limited to authorized users (given that it takes external action, rather than e.g. adding a label to the given issue). This is similar to most other commands we have today, so we don't need to worry too much about removal.

The remove command probably needs to take some form of token as well (perhaps the same playpen URL, and we can key by that in whatever database/filesystem/etc we're using to store these pens).

@JohnTitor
Copy link
Member

Currently, glacier checks if there are fixed ICEs every day with GHA so I think glacier remove command is unhelpful maybe.
And it'd be nice that glacier add submits a PR to glacier like autofix, then we(members) can review it on glacier, I think the behavior is useful enough to add ICEs to glacier.

@Mark-Simulacrum
Copy link
Member

I don't see what relevance there is that glacier checks every day on GHA to how useful the remove command is?

Regardless, it seems like the remove/add commands filing PRs (versus directly editing the repository) is reasonable. Then they can be permissionless, I imagine.

@JohnTitor
Copy link
Member

Hm, I cannot imagine the needs of remove but I won't argue anymore since at least you want that.

@Mark-Simulacrum
Copy link
Member

Does autofix remove fixed ICEs via PR automatically? That was my envisioned use case for the remove command but if that already happens then there's no point most likely.

@JohnTitor
Copy link
Member

Does autofix remove fixed ICEs via PR automatically?

Yeah, autofix does it.

@hellow554
Copy link
Author

So I will remove the remove command, because triagebot will open a new PR instead of commiting directy to the glacier repository?

@Mark-Simulacrum
Copy link
Member

That sounds correct, yes. So we'll just have the add command which for now takes a URL in quotes (i.e., @bot glacier add "URL").

@Elinvynia
Copy link
Contributor

Hello, what is the status of this PR? I'd be interested in helping out.

@Mark-Simulacrum
Copy link
Member

I believe someone could take the current PR and attempt to finish it, but beyond reviewing PRs I don't currently have the bandwidth to figure out the exact next steps here. Feel free to ping me on Zulip though if you have questions (#t-release/triagebot stream) and I can try to answer them

@hellow554
Copy link
Author

hey @Elinvynia I haven't developed this any further, but please feel free to take this! I hope this helps to start with the issue. I don't have time for it, sorry :(

@hellow554
Copy link
Author

Closing in favor of #526

@hellow554 hellow554 closed this May 12, 2020
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.

4 participants