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 client route for deleting remote links by global (and internal) id #1395

Merged
merged 3 commits into from Jun 29, 2022

Conversation

rynkk
Copy link
Contributor

@rynkk rynkk commented Jun 3, 2022

Took me a bit longer than expected :)

fixes #1300

The program was tested solely for our own use cases, which might differ from yours.


Jannik Meinecke jannik.meinecke@mercedes-benz.com on behalf of MBition GmbH.
https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

Licensed under BSD-2-Clause license

@studioj studioj added the feature label Jun 4, 2022
tests/tests.py Outdated Show resolved Hide resolved
@studioj
Copy link
Collaborator

studioj commented Jun 4, 2022

have a look at my PR pyproject.toml to fix the build issues
#1387

OR wait and rebase onto main :-)
A review would also be nice ☺️

I would also suggest to extend the examples and the docs

adehad
adehad previously approved these changes Jun 5, 2022
Copy link
Collaborator

@adehad adehad left a comment

Choose a reason for hiding this comment

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

Looks awesome to me too!

As @studioj says let's wait for the build to be fixed by the other PR before we merge this in.

@rynkk
Copy link
Contributor Author

rynkk commented Jun 7, 2022

Great! Glad I was able to help out. @studioj I implemented your suggestions :-)


Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@rynkk
Copy link
Contributor Author

rynkk commented Jun 9, 2022

@ssbarnea @adehad, on a different note: My company strongly suggests (requires) contributors to add copyright notices to code, if applicable.
However I see, that so far no copyright notice is included in the source files at all. Would it be fine for me to add the following snippet above the method body?

# Copyright (c) 2022 MBition GmbH
# Jannik Meinecke <jannik.meinecke@mercedes-benz.com>

Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@studioj
Copy link
Collaborator

studioj commented Jun 9, 2022

@ssbarnea @adehad, on a different note: My company strongly suggests (requires) contributors to add copyright notices to code, if applicable. However I see, that so far no copyright notice is included in the source files at all. Would it be fine for me to add the following snippet above the method body?

# Copyright (c) 2022 MBition GmbH
# Jannik Meinecke <jannik.meinecke@mercedes-benz.com>

Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH. Provider Information

Hey @rynkk what would this entail? The package is released under the "Simplified BSD License" would that method then be excluded from that license and be owned by Mercedes? Please enlighten me/us (I'm guessing @ssbarnea has a better idea what this means)

@studioj
Copy link
Collaborator

studioj commented Jun 9, 2022

@adehad I didnt follow the cloud tests but they are failing and it doesnt seem to be related ... or is that because this is a fork?
it doesnt seem to be related anyway

@rynkk
Copy link
Contributor Author

rynkk commented Jun 10, 2022

Hey @rynkk what would this entail? The package is released under the "Simplified BSD License" would that method then be excluded from that license and be owned by Mercedes? Please enlighten me/us (I'm guessing @ssbarnea has a better idea what this means)

@studioj, the license is entirely unaffected by the copyright (unless you want to change the license, then you need all copyright holders to agree afaik). The main reasons are visibility (for the company), traceability (for both parties) and reliability (for you).

However, after talking to my FOSS-coordinators, it would be perfectly fine to just use the AUTHORS.rst in the root of this project. No need to taint your previously untouched source files!


Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@adehad
Copy link
Collaborator

adehad commented Jun 10, 2022

@adehad I didnt follow the cloud tests but they are failing and it doesnt seem to be related ... or is that because this is a fork? it doesnt seem to be related anyway

yeah just the usual forks not having secrets access

@adehad
Copy link
Collaborator

adehad commented Jun 10, 2022

@rynkk regarding the copyright attribution, I am no expert so will chase Sorin on element.io about this.

I had a look at your company's foss repo and it sounds like you need to jump through some hoops with a CLA? Gave it a read and from Section 3.1 it seems like all copyright is given to the project if my understanding is correct, so I'm surprised you need to include a notice.

My understanding is, if we chose to, that our files will contain a copyright message which is generic and attributes to "all project contributors".
Not sure the most practical way to add your notice. I would have thought one can include it in the (squashed) commit message. We already include a link to the pull request in the merged commit so it would already reference your contribution sponsor/affiliation as you've included it in the description. So in that sense it is on record that it was added by who at what time and with what affiliation.
Not opposed to using the AUTHORS file, we can maybe add a section specifically to cover the CLA.

@rynkk
Copy link
Contributor Author

rynkk commented Jun 10, 2022

Section 3.1 it seems like all copyright is given to the project if my understanding is correct, so I'm surprised you need to include a notice

@adehad, actually all the copyright that I created is given to my FOSS Co, which in this case is MBition GmbH, a subsidiary of the Mercedes Benz Group (which is my direct employer). I must admit I am not an expert in this matter either and maybe I have misread and you are right.
Personally I'm fine with just the Authors-file. The CLA section probably cannot hurt, though, how do they say after all, Explicit is better than implicit. If only I could remember where I read that ;-)


Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@ssbarnea
Copy link
Member

The amount of code contributed is shorter that some bugs reported or comments I seen on github and I prefer not to introduce any noise in the code. Does your employer require you to add copyright notice on posts? I guess no.

Shortly, better to avoid adding anything as it would add noise at best and make project harder to maintain. At worse it would open can of worms (endless discussions) regarding licensing, at least now we one license and any contribution goes under it. KISS

Regarding visibility of contributions, the best place to advertise them is on https://github.com/pycontribs/jira/releases and that is automatic, as usernames are mentioned on each release.

On the other hand AUTHORS file is very outdated and I am considering replacing it with a link to https://github.com/pycontribs/jira/graphs/contributors, as it would prove too hard to maintain, as we get lost of contributors.

@rynkk
Copy link
Contributor Author

rynkk commented Jun 28, 2022

@ssbarnea that's fine by me. My employer is fine with whatever way the project uses to document contributers - as long as it actually documents them.


Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@adehad
Copy link
Collaborator

adehad commented Jun 28, 2022

@rynkk

Just confirming you are happy proceeding?
The commits will be squashed before merging, I will use your existing commit message:

Add client route for deleting remote links by global (and internal) id

___
Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH.
https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

I will do the same for your other contribution, and then close the PR relating to the AUTHORS.rst

@rynkk
Copy link
Contributor Author

rynkk commented Jun 28, 2022

Just confirming you are happy proceeding?

@adehad I am happy with proceeding. Thank you for helping!

I will do the same for your other contribution, and then close the PR relating to the AUTHORS.rst

Thank you!


Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@adehad
Copy link
Collaborator

adehad commented Jun 28, 2022

@rynkk could you merge/rebase to fix the merge conflicts

___
Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH.
https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md
@rynkk
Copy link
Contributor Author

rynkk commented Jun 29, 2022

@adehad, done.


Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.
Provider Information

@adehad adehad merged commit e811ac8 into pycontribs:main Jun 29, 2022
@adehad
Copy link
Collaborator

adehad commented Jun 29, 2022

Awesome, thanks again for your patience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add api-method to delete remotelinks
4 participants