search

metafacture / metafacture-core

Core package of the Metafacture tool suite for metadata processing.
https://metafacture.org
Apache License 2.0
69 stars 34 forks source link

Metafacture core 5.3.1rc #428

Closed dr0i closed 2 years ago

dr0i commented 2 years ago

This is the branch for testing metafacture-core release candidate 5.3.1. It follows up https://github.com/metafacture/metafacture-core/issues/397.

Based on the experience we've made with the release of metafacture-core 5.3.0 (which is deprecated resp. not recommended for usage) it was discussed with @blackwinter the need to better test release candidates before actually releasing them (this is also reflected now in the Maintainer Guidelines).

@blackwinter found the dependency upgrade of log4j:1.2.12' to log4j-core:2.14. problematic, so this is reverted in 36ed96920c70a0173abb75f39ed28ca8bbba7bab 462c8f80d50e70daeb6afdc13ca6a6bda6be4c1a.

I have tested 5.3.0 against lobid-resources and found an incompatibility which I reverted in 7c1ea04c6012fbea661a86d73e6c0ef8f8f53ff8.

Furthermore, I found out that if you are using a metamorph.xsd of your own and make use of FileMap you have to also update your locally metamorph.xsd (like the one at https://github.com/hbz/lobid-resources/blob/master/src/main/resources/schemata/metamorph.xsd):

-      <attribute name="separator" type="string" use="optional" default="\t">
+      <attribute name="separator" type="string" use="optional" default="&#09;">

As this will occur only quite rarely (is there anybody out there using a local duplicate of metamorph.xsd ?) and this is the result of a bug fix (d528ac973a7b6a1bb0b846a6d90950ace9553742) (in effect the default separator defined (falsely) in metamorph.xsd has had no effect at all) this "breaking" fix will be part of the release (or is anyone against this?) but the release would come with the here mentioned note.

blackwinter commented 2 years ago

Are there actual release artifacts to test? I can't find any 5.3.1-rc1 on Sonatype or GitHub.

found an incompatibility which I reverted in 7c1ea04.

That's 462c8f8 now, right?

blackwinter commented 2 years ago

refs/tags/metafacture-core-5.3.1-rc

IMO, release candidates should always be numbered.

dr0i commented 2 years ago

That's 462c8f8 now, right?

right

IMO, release candidates should always be numbered.

did so now

The snapshot can be consumed as feature-5.3.1-rc1-SNAPSHOT in your building system. Note to properly configure it, e.g. for maven add to your pom.xml:

<repositories>
    <repository>
        <id>oss.sonatype.org-snapshot</id>
        <url>https://oss.sonatype.org/content/repositories/snapshots</url>
        <releases>
             <enabled>false</enabled>
        </releases>
        <snapshots>
            <enabled>true</enabled>
        </snapshots>
    </repository>
</repositories>

See the (hopefully now) comprehensive how-to in the wiki.

dr0i commented 2 years ago

don't get why these changes were made in an RC branch.

hmhm. Wait, you are right! We should first merge this PR into master and then make an rc, right? Hm, but then how to review properly, because we want to test this PR within our programs.

Do you propose to first merge this PR into master (you have already given your +1 so we can do this right now) and then build an rc and open an issue to test this? (+1 from me)

blackwinter commented 2 years ago

We should first merge this PR into master and then make an rc, right?

Yes, that would be my suggestion.

Hm, but then how to review properly, because we want to test this PR within our programs.

To me, those are different things. Want to review a pull request/feature branch? Publish a preview release (feature-<branch>). Want to review an upcoming release? Publish a release candidate (from the main/release branch; ideally, without the feature- prefix). I admit that this comes with a bit of overhead as long as the releases have to be performed manually.

you have already given your +1 so we can do this right now

There are multiple reviewers. Shouldn't all of them get a chance to test the release artifacts?

dr0i commented 2 years ago

There are multiple reviewers. Shouldn't all of them get a chance to test the release artifacts?

But I understand your proposal to first merge into master and then make release artifacts and open an issue to let them test the release artifacts?

blackwinter commented 2 years ago

But I understand your proposal to first merge into master and then make release artifacts and open an issue to let them test the release artifacts?

Yes, but the question is what they should test. This PR (they're listed as reviewers after all) or the RC (which, for now, coincides with this PR)?

dr0i commented 2 years ago

Thx for the clarification , also thx for the gradle config in the wiki!

It's agreed for normal PRs that one +1 is sufficient, so I've merged this PR. Opened a new issue for testing the already buildt release artifacts at #429, reassigning the reviewers in question.

blackwinter commented 2 years ago

OK, thanks! That should make things a little clearer.