add test for issue #453 - fix is done in neko

rbri commented 5 months ago

use 4.2.0-SNAPSHOT

rbri commented 5 months ago

The check fails because the pom requires the entry for the snapshot repo.

see https://github.com/HtmlUnit/htmlunit?tab=readme-ov-file#maven-1 for more

rbri commented 5 months ago

@davewichers @spassarop sorry for the lazy comment, this is fixed with the latest neko snapshot

spassarop commented 5 months ago

LGTM. My only doubt is we should have the snapshots repo on the pom.xml or just wait for the 4.2.0 release. I guess it is no problem but I am not used to work with Java public projects and snapshots. @davewichers is it ok? If it is it can be merged.

kwwall commented 5 months ago

Just an FYI, but in my experience with the last 3 companies I've worked for generally would not allow SNAPSHOT dependencies (including transitive ones) unless perhaps it was for an emergency patch situation (which is hardly the case here). Some of them would allow RC (release candidate) dependencies though. Maybe ask if that's at least a possibility.

On Sat, Jun 1, 2024, 6:25 PM Sebastián Passaro @.***> wrote:

LGTM. My only doubt is we should have the snapshots repo on the pom.xml or just wait for the 4.2.0 release. I guess it is no problem but I am not used to work with Java public projects and snapshots. @davewichers https://github.com/davewichers is it ok? If it is it can be merged.

— Reply to this email directly, view it on GitHub https://github.com/nahsra/antisamy/pull/454#issuecomment-2143611852, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG4FP2FGUQZOTFDIRF3ZFJC5JAVCNFSM6AAAAABILCJPHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGYYTCOBVGI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-kevin

davewichers commented 5 months ago

@spassarop - just use this MR to TEST, we will NOT merge the pom change as we won't use a snapshot dependency in a release. Please test this and let me know if it works and you are OK with everything. If you are, please ask @rbri to release a new version of his library so we can upgrade and do a new release of AntiSamy with the new version.

rbri commented 5 months ago

Just an FYI, but in my experience with the last 3 companies I've worked for generally would not allow SNAPSHOT dependencies (including transitive ones) unless perhaps it was for an emergency patch situation (which is hardly the case here). Some of them would allow RC (release candidate) dependencies though. Maybe ask if that's at least a possibility.

Hi @spassarop, @davewichers, @kwwall maybe there is some misunderstanding... ;-)

The idea of this PR is NOT to force you to use the SNAPSHOT build in the release!

The idea is for projects having this kind of close dependencies (you need fixes in neko to fix issues in antisamy)

make a fix in neko, include some tests that proves the fix
build a snapshot version of neko
make a PR for antisamy including a test that shows that this fixes the problem in antisamy
- this has to point to the neko snapshot to pass the ci
merge the PR and make a snapshot build of antisamy - you can deploy snapshot build depending on snapshot builds to sonatype
ask the reporter to test if the (published) snapshot solvers the problem
if yes coordinate release build of the dependent lib
switch to the release build an proof that the antisamy test suite still passes
publish an antisamy release depending on the neko release (sonatype does not accept release builds depending on snapshots)

kwwall commented 4 months ago

@rbri wrote:

Just an FYI, but in my experience with the last 3 companies I've worked for generally would not allow SNAPSHOT dependencies (including transitive ones) unless perhaps it was for an emergency patch situation (which is hardly the case here). Some of them would allow RC (release candidate) dependencies though. Maybe ask if that's at least a possibility.

Hi @spassarop, @davewichers, @kwwall maybe there is some misunderstanding... ;-)

The idea of this PR is NOT to force you to use the SNAPSHOT build in the release!

The idea is for projects having this kind of close dependencies (you need fixes in neko to fix issues in antisamy)

make a fix in neko, include some tests that proves the fix

build a snapshot version of neko

You've already done this part, right?

* make a PR for antisamy including a test that shows that this fixes the problem in antisamy
  * this has to point to the neko snapshot to pass the ci

For AntiSamy to use org.htmlunit:neko-htmlunit:4.2.0-SNAPSHOT, you still need to publish it to a Maven repo, correct? Only instead of publishing it to OSSRH, you are apparently proposing to push it to the OSS Sonatype snapshots, https://s01.oss.sonatype.org/content/repositories/snapshots/ ?

That's what I don't understand. If @jeetu22 or anyone else needs to test this against their proprietary corporate code, there is a fair chance that they will not be able to access this by default from their corporate firewalls.

And while this may not affect @jeetu22, it may affect others who wish to test it.

So rather than pushing out a SNAPSHOT to that Sonatype snapshot repo, why not mark this an RC1 release candidate and drop it in OSSRH as normal? That would allow AntiSamy to create a PR that uses org.htmlunit:neko-htmlunit:4.2.0-RC1and result in a lot more developers being able to test it without jumping through hoops.

* merge the PR and make a snapshot build of antisamy - you can deploy snapshot build depending on snapshot builds to sonatype

And AntiSamy would release 1.7.6-RC1 in OSSRH rather than 1.7.6-SNAPSHOT in the OSS Sonatype snapshot repo. That doesn't seem like a lot of additional work to me, but more importantly it drastically lowers the bar to all Neko and AntiSamy clients who wish to test it. ESAPI has released several release candidates like this before. That's sort of how the process works. SNAPSHOT implies more like an ongoing, potentially very unstable development stream. RC# implies a succession of slightly more stable alpha or beta test releases. (Although, some OSS projects seem to use '-M#' suffixes for this, instead of 'RC'; I'm not sure what the 'M' stands for though.)

This also simplifies things from AntiSamy's proposal as they don't need to temporarily change the section and then remember to change it back in their pom.xml when they are ready to do the stable 1.7.6 release.

* ask the reporter to test if the (published) snapshot solvers the problem
* if yes coordinate release build of the dependent lib
* switch to the release build an proof that the antisamy test suite still passes
* publish an antisamy release depending on the neko release (sonatype does not accept release builds depending on snapshots)

I think we're on the same page EXCEPT, I'm advocating using Release Candidates instead of Snapshots so OSSRH can still be used. That's going to be a lot less hassle for all of the Neko and AntiSamy clients even it it potentially is a bit of additional extra re-work for you and the AntiSamy team. But IMO, it's the right thing to do.

Just my $.02. Neither of these are my project and I do all my ESAPI work from my personal laptop, so I'm not sitting behind a corporate firewall / filtering proxy. (Besides, this doesn't affect ESAPI, so there's no urgency of testing it before the official 1.7.6 AntiSamy release.) But some of your clients potentially will be negatively affected if SNAPSHOTs are used, and I think that's something to consider.

spassarop commented 4 months ago

Um, what Kevin says sounds reasonable. Again, Java public code management, can't give an opinion.

On the other side, the test is fine, I reviewed it. So you can go on with how you deem appropriate on the release :)

@rbri Thanks again for your efforts and taking the time to get involved in these kind of fixes and discussions even though it is not your own project, same for @kwwall ;)