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

Introduce issue templates #549

Merged
merged 9 commits into from Oct 29, 2019
Merged

Introduce issue templates #549

merged 9 commits into from Oct 29, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Oct 28, 2019

GitHub has updated issue template interface, let's use them.

GitHub has updated issue template interface, let's use them.
@ilammy ilammy requested a review from karenswry October 28, 2019 14:53
@vixentael
Copy link
Contributor

You suggest to remove "openssl/boringssl/i don't know" question?

Co-Authored-By: vixentael <vixentael@users.noreply.github.com>
@ilammy
Copy link
Collaborator Author

ilammy commented Oct 28, 2019

You suggest to remove "openssl/boringssl/i don't know" question?

Yeah. I’ve looked through recent bug reports and don’t see any where this actually mattered. Either the issue was entirely unrelated to the backend, or the submitting person experienced an issue with specific backend and explicitly submitted an issue about that. I think, in general case it will be enough to know the platform and whether Themis has been installed via the package manager or compiled from source to triage the issue.

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 28, 2019

And while we’re here, let’s add a template for pull requests as well. I don’t think that we have that many PRs to mandate a formal structure, but it would be nice to have a check list of important things: like, don’t forget to write unit tests, update changelog, eat your veggies, etc.

.github/PULL_REQUEST_TEMPLATE/pull_request.md Outdated Show resolved Hide resolved
It should be obvious from the displayed integration, and merges are
usually safeguarded against accidentally merging a branch that has
failed CI.
@shadinua
Copy link
Contributor

Probably we should add also architecture field to Environment block. OS name may be the same, but architectures may be different.

@vixentael
Copy link
Contributor

vixentael commented Oct 28, 2019

Probably we should add also architecture field to Environment block.

maybe just as simple as

[ ]  x32
[ ]  x64

but it's valid only for desktop/server side platforms

@shadinua
Copy link
Contributor

but it's valid only for desktop/server side platforms

[ ] x32
[ ] x64
[ ] other: ______

@vixentael
Copy link
Contributor

[ ] x32
[ ] x64
[ ] mobile

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 29, 2019

I’m not sure that arch will be of significant help for bug reports. It certainly can be important (like that one time, @vixentael 😉), but I doubt that we’ll have a constant stream of diverse architecture-specific issues. It’s not a safety check list written in blood for us to include every minor detail and turn it into 50-item questionnaire for the users to fill out.

If anything, instead of that I’d rather add a separate Device entry for mobile users, where the architectures are the most diverse. This will be more useful indicator of whether we can reproduce the environment precisely or will have to go with "idk, works on my phone *shrug*, could you please double-check everything?"

@vixentael
Copy link
Contributor

I'm not sure if @shadinua actually wanted to see device model rather than x32/x64 CPU architecture.

I agree that having 50-items questionnaire is "too much", but I'd rather want to understand x32/x64.

Suggestion: change Device to Hardware: [x32/x64, or mobile device version]

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 29, 2019

Well, technically on x86 it’s possible to run 32-bit kernel on 64-bit hardware, or compile and run 32-bit applications with 64-bit kernel...

Anyhow, this is pedantry which does not get us anywhere. We don’t have many bug reports filed, so I don’t have any data to argue which template would be ‘the best’. How about the following:

Hardware: [32-bit/64-bit, or mobile device name and version]

is everyone happy with this wording?

P.S. I hate how the internet is filled with x32, x86, i386, i686, IA-32, x64, x86_64, AMD64, which are similar or sometimes the same, but not quite the same, and everybody (including me) has an opinion which one is the correct spelling for the architecture. And on top of that, Linux has x32 which is an obscure ABI, not quite what you think it is.

@vixentael
Copy link
Contributor

Hardware: [32-bit/64-bit, or mobile device name and version]

yes, let's do it

@ilammy ilammy merged commit ae25b16 into master Oct 29, 2019
@ilammy ilammy deleted the templates branch October 29, 2019 18:00
@ilammy
Copy link
Collaborator Author

ilammy commented Oct 29, 2019

Hm... the pull request template does not work for some reason, but we’re doing it according to the docs 🤔

@vixentael
Copy link
Contributor

Maybe, we should name it pull_request_template.md, as shows in this doc?

Github shows this tab for feature request / bug templates:
Screen Shot 2019-10-30 at 12 44 47 AM

but i don't similar alert for PR template:
Screen Shot 2019-10-30 at 12 44 04 AM

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 30, 2019

It seems that we need to have it under .github/pull_request_template.md, not in a subdirectory, for it to act as a default template for pull requests. The ones in PULL_REQUEST_TEMPLATE can be used only with query parameters (e.g., to insert a link [Send us a pull request!] somewhere).

I’ll fix that up in master branch (without a PR).

@vixentael vixentael added the docs 📚 Documentation, both offline and online label Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs 📚 Documentation, both offline and online
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants