cEP-0015.md: corobo enhancement #113
Conversation
cEP-0015.md
Outdated
| ``` | ||
| > corobo stop | ||
| corobo has been sent to safe-mode by @Makman2 | ||
| ``` |
There was a problem hiding this comment.
Line contains following spacing inconsistencies:
- Trailing whitespaces.
Origin: SpaceConsistencyBear, Section: spacing.
The issue can be fixed by applying the following patch:
--- a/tmp/tmp592ed8pn/cEP-0015.md
+++ b/tmp/tmp592ed8pn/cEP-0015.md
@@ -90,4 +90,4 @@corobo stop
corobo has been sent to safe-mode by @Makman2
-+
|
A lot needs to be added to this cEP at the moment. I'll be adding more details in the coming days. |
cEP-0015.md
Outdated
| -------- | ||
|
|
||
| This cEP describes the details of enhancement of [corobo](https://github.com/coala/corobo) in terms of security, tests, configurability and | ||
| the new plugins that are to be added to corobo as part of the [GSoC Project](https://summerofcode.withgoogle.com/projects/#6603667076022272). |
There was a problem hiding this comment.
I'd suggest to improve the project description on GSoC website as well, while you can.. maybe after the end of community bonding period.
There was a problem hiding this comment.
I didn't know we can update the description :o Thank's for informing, its done 👍
cEP-0015.md
Outdated
| coala/coala-bears#2326 [Add .pytest_cache to .gitignore] | ||
| ``` | ||
|
|
||
| ### Safe mode |
There was a problem hiding this comment.
Define safe mode, what will be disabled, what will be accessible, etc.
There was a problem hiding this comment.
@meetmangukiya I had been thinking about this one.
My idea was that this command will deactivate a list of plugins which we provide in case of emergency situations, but will we need something like this after security hardenings?
There was a problem hiding this comment.
There's already plugin management plugin built into errbot, which can be used to activate and deactivate a specified plugin. If deactivation is the only feature, I don't think we need safe mode.
cEP-0015.md
Outdated
| 1. invite | ||
| 2. assign | ||
| 3. unassign | ||
| 4. mark wip|pending |
There was a problem hiding this comment.
Actually all four of this commands are part of LabHub plugin, so not a List of plugins per se. No other plugins that are coala specific, are you sure?
There was a problem hiding this comment.
coatils for one is coala specific and will remain coala specific, maybe move such specific plugins to a different directory and prevent them from loading for non-coala setup in Dockerfile, maybe?
There was a problem hiding this comment.
Other coala specific plugins are:
- answers -- we can maybe make this coala independent and make it a generic docs based answering plugin, will require extensive refactoring
- coala_lowercase_c - coala specific only, can't adapt
- explain -- hardcoded coala explanations
- searchdocs -- ?
There was a problem hiding this comment.
And regarding corobo explain <term> , I think this one is already generalized.
One can simply just add a template in dir plugins/templates/explain/ to add new explanations of his choice?
or we can make the user setup a variable EXPLAIN_DIR , which will contain locations to the explaination templates?
There was a problem hiding this comment.
maybe have two sub-directories explain/generic and explain/<org name> , so this repo can be a depot for explains of other orgs.
There was a problem hiding this comment.
For searchDocs plugin, currently API_DOCS and USER_DOCS URLs are constants for coala.
I suggest, we can make it generic by making the orgs setup their custom search URL/URLs in config file.
There was a problem hiding this comment.
@meetmangukiya creating generic docs based answering plugin will require changes in the answers microservice itself as the docs are parsed there. How can we go about making this generic for every organization o.O ?
cEP-0015.md
Outdated
| Abstract | ||
| -------- | ||
|
|
||
| This cEP describes the details of enhancement of [corobo](https://github.com/coala/corobo) in terms of security, tests, configurability and |
There was a problem hiding this comment.
cEP-0015.md
Outdated
|
|
||
| 5. Prevent corobo being used to spam a room. | ||
|
|
||
| 6. Force newcomers to finish one issue first. |
There was a problem hiding this comment.
can you explain this a bit?
cEP-0015.md
Outdated
|
|
||
| 7. Require newcomers to find a newcomer issue to work on before they are | ||
| invited to join the organization. | ||
|
|
There was a problem hiding this comment.
maybe even ban newcomers for a day or two, if they violate too many rules given in the newcomer's guide
There was a problem hiding this comment.
Won't that be too harsh :P ?
The developers/maintainers will invite them and assign the issue only after they find one to work on.
There was a problem hiding this comment.
thats why just for a day or two, asking them nicely to again go through the newcomer's guide and some git tutorials and such
There was a problem hiding this comment.
if they are doing so many things wrong, means they have taken this lightly and haven't gone through the provided content. also asking too many googlable questions may count to that
There was a problem hiding this comment.
I agree with your point but that might scare them away from contributing :P
We can definitely add a command to warn them about not following the guidelines.
I think we'll need some discussion on this point.
There was a problem hiding this comment.
as I said its completely your call on that 👍
There was a problem hiding this comment.
We can add a scarscatic explaination for corobo explain google :P
cEP-0015.md
Outdated
|
|
||
| 6. Force newcomers to finish one issue first. | ||
|
|
||
| 7. Require newcomers to find a newcomer issue to work on before they are |
There was a problem hiding this comment.
so like looking through newcomer issues and checking for possibilities like:
- no ones assigned to it
- no one claimed it in the last week
- no one pushed changes or was even active on gitter since the last week, in which case atleast newcomer issues can be reassigned as they don't require more work than that, also pinging newcomers of their unassignment that they havent worked for so and so time on a newcomer issue and it will be reassigned, more time gap for low issues
- discarding issues which have users waiting in the queue wanting to get assigned both from gitter and comments on GitHub
this stuff would be cool af,
would love to see it actually working
cEP-0015.md
Outdated
| coala/coala-bears#2376 [Fix invalid language setting] | ||
| coala/coala-quickstart#128 [add .DS_Store to .gitignore] | ||
| coala/coala-bears#2326 [Add .pytest_cache to .gitignore] | ||
| ``` |
There was a problem hiding this comment.
would be great if you can stuff some more implementation details into this 😉
There was a problem hiding this comment.
Is there an issue about this feature?
If so, please mention it.
There was a problem hiding this comment.
No there is no issue yet, but I'll create one and mention it here.
There was a problem hiding this comment.
not about this feature, kind of comparing this cEP in general to others
There was a problem hiding this comment.
coala/corobo#528 has been created.
IMO this cEP, and probably even this project, should not have new features which are not related to "security, tests and configurability". Your project summary in GSoC doesnt mention new features. Your GSoC proposal did mention adding this feature, but it definitely isnt relevant to the stated scope of this cEP.
It might be still included in your milestone tasks, but it may be dropped if the project team agrees that other pending work is more important. e.g. one of the critical components of this project takes more time than expected. This feature is useless if the corobo hardening isnt completed as best as is possible.
Anyone could add new features like this. It doesnt require a GSoC student to do it. GCI students last year built bigger more complicated plugins in less than three days. If you dont build it this GSoC, no drama .. a newcomer or GCI student will build it by the end of the year.
Also I feel that it would be inappropriate to build more GitHub-only features into corobo. corobo should exclusively use IGitt, and all plugins should work in all hosters supported by IGitt, unless it is functionality which is only supported by one hoster and it is really critical that it is available in corobo.
fwiw, another 2018 GSoC project will be providing a GitHub+GitLab issue search tool.
There was a problem hiding this comment.
Okay, I'll remove this section since it is out of scope for this project :)
cEP-0015.md
Outdated
| ### How to improve existing tests and infrastructure? | ||
|
|
||
| This will involve making changes upstream in | ||
| [Errbot](https://github.com/errbotio/errbot/) and extend the existing testing |
There was a problem hiding this comment.
and extend testing ...
What changes to errbot do you have in mind, other than testing?
Details please, or dont mention it here.
Fixing their testing framework, and implementing those changes, is priority no 1 for your project. corobo becomes abandonware if you dont get testing working. And likely your project is failed after coding period 1 if you haven made significant inroads into solving that.
Need to see a lot more details about that one specifically.
cEP-0015.md
Outdated
| 1. Make all LabHub commands except invite require being a member of the | ||
| organization. | ||
|
|
||
| 2. Add ability to ban from all gitter rooms at once. |
There was a problem hiding this comment.
this isnt security.
the only way that helps security is if being in a gitter room gives the user access to sensitive/restricted operations, which sounds like a very bad idea as we cant control people joining the room with lots of accounts, either themselves or getting their friends to help coordinate an attack.
this is access control, but the goal of it is to prevent spamming.
There was a problem hiding this comment.
Okay, I'll reframe the structure of this cEP and count this in as a measure to prevent spamming than security.
cEP-0015.md
Outdated
| ### Why improve security? | ||
|
|
||
| Security has been one of the major concerns due to some past experiences. | ||
| We want to provide access controls based on user’s GitHub team memberships. |
There was a problem hiding this comment.
Firstly We doesnt belong in a spec/proposal.
But more importantly, this starts the proposal re security on the wrong foot.
Here you are dictating implementation details in the Why?. That eliminates the need for requirements gathering, and usually ends up with failure.
The sole purpose of corobo is to empower coala members to do things according to the orgs defined roles, giving them more power than is possible with GitHub's and GitLab's limited (and often bizarre) access control mechanisms.
As an extremely open org (and sadly you havent experienced it as much as the previous GSoC students due to the aforesaid problems), any access control is inherently bad and must be justified as a necessary evil.
corobo is a balancing act of giving org members rights to do whatever they need to do, vs preventing them from causing mayhem.
We havent got that balancing act right yet, hence this project ;-)
GitHub team memberships are a means to an end.
The most important requirement is allocation of issues to people, and for people's first issue, that means we have no idea (zero trust) in the person being given org rights.
However team(org) membership comes with serious problems. Foremost is that any member of the org has rights given by GitHub which we probably dont want them have, and have little counter-measures or tested procedures to deal with. This is where the testability aspect comes in. 99.99% of the time we dont need these counter-measures. But the one day we do need to react, we are going to be very reliable on counter-measures which havent been used for months or years. A reliable test-suite for counter-measures gives us confidence that we can grant newcomers rights even if we dont yet trust them, because we are able to react effectively enough to halt bad actors soon enough that the cost/benefit is stacked in favour of given the rights.
Read through https://help.github.com/articles/permission-levels-for-an-organization/ and https://help.github.com/articles/repository-permission-levels-for-an-organization/ to see how screwed we are if a newcomer wants to be disruptive.
One of the recently found problems is that people can do a merge on their own repos which closes issues in the org, if they use Closes coala/coala#123. Maybe that is only org members? We havent checked that yet, but that would be one of the more extremely disruptive problems. (an abusing person might batch close all issues..)
See https://gitlab.com/coala/roles/ for a possible way to avoid GitHub team membership for newcomers who havent yet been found to be trustable. See https://docs.gitlab.com/ee/user/permissions.html for a much saner set of minimums. But still not granular enough, and contains a few concerning permissions. This would mean we wouldnt be able to assign GitHub issues to them.
Another option is to use another org like 'coala-newcomers' for the newcomers, so they have no rights in the 'coala' org, but then we cant assign issues to them.
Another option is to not use GitHub or GitLab for users until they have been vetted, but use a user list stored somewhere else which only has the access which corobo has configured for them, and no implicit rights given by the repo hoster. Again, this would mean we cant assign issues to them on GitHub.
Notice a theme here ... assigning issues is our process problem wrt GitHub. The GitMate project has a few possible solutions which havent been road tested, and they wont be perfect. The corobo project needs to also have solutions for this problem, and again they will be limited workarounds. It is a significant enough problem, and coala isnt the only org with this problem, that having multiple different solutions are needed, and may need to be used together to achieve the right balance, or at least different orgs will want to use different solutions, and coala's mission includes building tools that other orgs can use, so offering multiple solutions empowers those orgs to find the one that suits them best...
There was a problem hiding this comment.
https://help.github.com/articles/closing-issues-using-keywords/
It says Including Closes example_user/example_repo#76 will close the referenced issue in that repository, provided you have push access to that repository.
Only maintainers/owners have push/write access, it won't be possible for newcomers to close issues?
people can do a merge on their own repos which closes issues in the org, if they use Closes coala/coala#123. Maybe that is only org members? We havent checked that yet, but that would be one of the more extremely disruptive problems
nvzard
force-pushed
the
nvzard/corobo_cep
branch
2 times, most recently
from
48a58db
to
4627d15
Compare
cEP-0015.md
Outdated
| class DefaultConfigMixin: | ||
| @property | ||
| def _default_config(self) -> Optional[Mapping]: | ||
| if 'DEFAULT_CONFIGS' in self.bot_config and self.name in self.bot_config['DEFAULT_CONFIGS']: |
cEP-0015.md
Outdated
| 'LabHub': { | ||
| 'GH_TOKEN': os.getenv('GITHUB_TOKEN'), | ||
| 'GL_TOKEN': os.getenv('GITLAB_TOKEN'), | ||
| 'GH_ORG_NAME': os.getenv('OPENHUB_TOKEN'), |
There was a problem hiding this comment.
GH_ORG_NAME = OPENHUB_TOKEN ? ;-)
nvzard
force-pushed
the
nvzard/corobo_cep
branch
from
e1fcddc
to
9530c0c
Compare
cEP-0015.md
Outdated
| @property | ||
| def _default_config(self) -> Optional[Mapping]: | ||
| if (self.bot_config.DEFAULT_CONFIG and self.name | ||
| in self.bot_config.DEFAULT_CONFIG): |
There was a problem hiding this comment.
Line contains following spacing inconsistencies:
- Tabs used instead of spaces.
Origin: SpaceConsistencyBear, Section: spacing.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpd0vhbsct/cEP-0015.md
+++ b/tmp/tmpd0vhbsct/cEP-0015.md
@@ -180,7 +180,7 @@
@property
def _default_config(self) -> Optional[Mapping]:
if (self.bot_config.DEFAULT_CONFIG and self.name
- in self.bot_config.DEFAULT_CONFIG):
+ in self.bot_config.DEFAULT_CONFIG):
return self.bot_config.DEFAULT_CONFIG[self.name]
def __init__(self, bot, name=None) -> None:
nvzard
force-pushed
the
nvzard/corobo_cep
branch
from
9530c0c
to
08c97c8
Compare
|
@gitmate-bot rebase |
|
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
gitmate-bot
force-pushed
the
nvzard/corobo_cep
branch
from
08c97c8
to
bc093cc
Compare
|
Automated rebase with GitMate.io was successful! 🎉 |
|
@gitmate-bot rebase |
|
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
gitmate-bot
force-pushed
the
nvzard/corobo_cep
branch
from
bc093cc
to
d20be0e
Compare
|
Automated rebase with GitMate.io was successful! 🎉 |
|
ack d20be0e |
|
@gitmate-bot ff |
|
Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently |
|
Automated fastforward with GitMate.io was successful! 🎉 |
Closes #110