search

sonatype / ossindex-public

Sonatype OSS Index - Public
Apache License 2.0
6 stars 9 forks source link

Implement a marshaller based on Jackson #6

Closed sschuberth closed 4 years ago

sschuberth commented 5 years ago

Please have a look at the individual commit messages for the details.

sschuberth commented 4 years ago

@jdillon, any ETA how much longer the review is going to take you?

jdillon commented 4 years ago

@sschuberth Sorry for the delay, a few things have come up internally that have taken precedence. The throws IOException part I've already merged, though its not yet published yet. The Jackson part I think needs a bit more consideration as presently adding another dep like Jackson to that module may cause other integrations to break. Though it may be made optional, or it may make sense to consider an additional module for client extensions like this.

Anyways, i've not forgotten about this, just haven't had time... but should get to it fairly soon I hope :-)

jdillon commented 4 years ago

The next release will have an optional JacksonMarshaller based on what you have provided here. Will probably reconsider the structure of the api/client bits in the future.

sschuberth commented 4 years ago

Hmm. Your commit, which is based on my work, does not properly maintain authorship nor does it give proper attribution (no, the "#6" reference to this PR is not enough). I would have preferred either you instructing me to do any required changes in the PR discussion, or merging my PR as-is and doing any required changes yourself on top.

Now, for anyone looking only at the Git history, it's unclear that I'm the original author of these changes. Which seems pretty insensible for a company which makes money with products in the OSS compliance tooling area, and it leaves the bad impression that this is just one of these "fake" OSS projects from commercial vendors which they publish for marketing reasons, but are not really willing (because they only mirror a variant of their closed source version) or able (due to a missing process) to accept community contributions.

jdillon commented 4 years ago

@sschuberth,

My apologies, I did not mean too offend or obscure the origin of this commit. I believe the origin of the change is fairly clear with the comment and link to the PR and discussion.

I believe I prematurely consumed your change without first checking your CLA status, and that may actually warrant removing the change until a CLA has been filed.

If you have not previously signed our CLA, please do sign this:

https://sonatypecla.herokuapp.com/sign-cla

I am sorry you do not agree with the means in which your change was applied. We will revisit our process for consuming changes to see if we can make any improvements in this area.

Best I can do presently however is to add a contributor attribution in the pom.xml file after ensuring that you have signed the CLA or remove the change.

sschuberth commented 4 years ago

I believe the origin of the change is fairly clear with the comment and link to the PR and discussion.

As mentioned, I disagree. There's a reason why Git makes a difference between the author and the committer, and I'm not the author of these commits anymore.

I believe I prematurely consumed your change without first checking your CLA status

I wasn't even aware that signing a CLA is necessary! Neither the README.md nor any CONTRIBUTING.md mentions this. If signing a CLA is required, I urge you to add an according check e.g. via https://github.com/cla-assistant/cla-assistant. Or better yet, use the DCO and https://github.com/probot/dco instead.

Anyway, if I knew a CLA needs to be signed, I wouldn't have contributed. I need to make up my mind what to do now that the contribution already happened.

We will revisit our process for consuming changes to see if we can make any improvements in this area.

Thank you, and yes please, your process needs to be revisited and changed so that authorship in Git history is maintained.

jdillon commented 4 years ago

@sschuberth,

Sorry for the trouble and lack of clarity on our process. I have removed the change and we will be looking into adding documentation and clarity over the required CLA shortly.

sschuberth commented 4 years ago

Ok, I see that the change has been reverted, not removed (from history), but I acknowledge that's the best we can do for now without force-pushing to master and thus breaking everybody's local copies.

sschuberth commented 4 years ago

we will be looking into adding documentation and clarity over the required CLA shortly.

@jdillon, and ETA when "shortly" will be, as by now 7 months have passed?