Default revision #5610

edmundmiller · 2024-12-13T16:46:13Z

Need to clean up this PR a bit, and test it out in a real-world, not just in tests.

netlify · 2024-12-13T16:46:32Z

❌ Deploy Preview for nextflow-docs-staging failed. Why did it fail? →

Name	Link
🔨 Latest commit	`d1ebdcc`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/6762d01fa509ae00089d8654

- Implemented `getDefaultRevision()` method in `Manifest.groovy` to retrieve the default revision. - Updated `AssetManager.groovy` to compare the current revision against the default revision instead of the default branch during checkout.

…efaultRevision scenarios - Added tests to verify behavior of AssetManager when handling defaultBranch and defaultRevision configurations. - Implemented scenarios for defaultBranch set to 'master', defaultRevision specified, and version as defaultRevision. - Included tests for cases with no defaultBranch and fallback mechanisms to ensure correct defaults are applied. - Ensured that commit hashes can be used as defaultRevision and validated the handling of version tags in absence of defaultBranch. - Marked several tests as PendingFeature for future implementation. refactor: Remove PendingFeature annotations from AssetManagerTest - Removed @PendingFeature annotations from multiple test cases in AssetManagerTest.groovy. - This change indicates that the tests are now fully implemented and ready for execution. - The tests cover scenarios for defaultBranch and defaultRevision configurations, ensuring comprehensive validation of AssetManager behavior.

- Removed @PendingFeature annotations from multiple test cases, indicating they are fully implemented. - Added comprehensive tests for AssetManager's handling of defaultBranch and defaultRevision configurations. - Implemented scenarios for various configurations, including defaultBranch set to 'master', specific defaultRevision, and version as defaultRevision. - Validated behavior for cases with no defaultBranch and ensured correct fallback mechanisms. - Enhanced tests for resolving script names and managing repository providers across different platforms.

…onsolidating configurations - Removed several test cases that were redundant or overly complex, focusing on essential scenarios for AssetManager's behavior. - Consolidated Git configuration tests into static final strings for better readability and maintainability. - Streamlined the setup process for tests, ensuring clarity in the initialization of AssetManager instances. - Enhanced the clarity of the remaining tests by improving comments and structure, making it easier to understand the purpose of each test case.

tom-seqera · 2025-01-21T09:09:47Z

modules/nextflow/src/main/groovy/nextflow/config/Manifest.groovy

@@ -55,11 +55,23 @@ class Manifest {


    String getDefaultBranch() {
-        target.defaultBranch
+        target.defaultBranch ?: 'master'


A git repository has its own concept of a 'default' branch, which is usually master or main - but can be absolutely anything. If the manifest doesn't specify a default branch/revision, we want to use the default branch set in git.

So, rather than assuming 'master', this method needs to return null if no value is specified in the manifest file. This allows the logic in AssetManager.getDefaultBranch() to interrogate git's default branch.

tom-seqera · 2025-01-21T09:12:48Z

modules/nextflow/src/main/groovy/nextflow/config/Manifest.groovy

+     * @return The revision to use
+     */
+    String getDefaultRevision() {
+        target.defaultRevision ?: getVersion() ?: getDefaultBranch()


I don't think 'version' should be in this list at all.

'defaultRevision'/'defaultBranch' are pointers to a (potentially) different branch/revision from the one checked out.

Version, if set, would usually represent the version of the checked out branch/revision, so I think including it in the list effectively means that whichever branch is checked out will become the default. It seems like this could result in surprising behaviour. Probably better only to rely on manifest fields which are explicitly related to defaults.

tom-seqera · 2025-01-21T09:23:47Z

modules/nextflow/src/main/groovy/nextflow/config/Manifest.groovy

+
+    /**
+     * Checks if the current version is a development version
+     * @return true if version ends with 'dev' or '-dev', false otherwise
+     */
+    boolean isDevelopmentVersion() {
+        def version = getVersion()
+        if (!version) return false
+        return version.endsWith('dev') || version.endsWith('-dev')
+    }
+
+    /**
+     * Checks if the current version is a release candidate
+     * @return true if version contains '-RC', false otherwise
+     */
+    boolean isReleaseCandidate() {
+        def version = getVersion()
+        if (!version) return false
+        return version.contains('-RC')
+    }
+
+    /**
+     * Checks if the current version is a hotfix
+     * @return true if version contains '-hotfix', false otherwise
+     */
+    boolean isHotfix() {
+        def version = getVersion()
+        if (!version) return false
+        return version.contains('-hotfix')
+    }
+
+    /**
+     * Validates if a version string is a valid semantic version
+     * @param version The version string to validate
+     * @return true if valid, false otherwise
+     */
+    boolean isValidVersion(String version) {
+        if (!version) return false
+        // Matches standard versions like 1.0.0, release candidates like 1.0.0-RC1, development versions like 1.0.0dev, and hotfixes like 1.0.0-hotfix
+        version ==~ /^\d+\.\d+\.\d+(-RC\d+|-dev|dev|-hotfix)?$/
+    }
+
+    /**
+     * Compares two version strings
+     * @return -1 if v1 < v2, 0 if v1 == v2, 1 if v1 > v2
+     */
+    private int compareVersions(String v1, String v2) {
+        def v1Parts = v1.tokenize('.')
+        def v2Parts = v2.tokenize('.')
+
+        for (int i = 0; i < Math.min(v1Parts.size(), v2Parts.size()); i++) {
+            def v1Part = v1Parts[i].replaceAll(/(-RC\d+|-dev|dev|-hotfix)$/, '') as int
+            def v2Part = v2Parts[i].replaceAll(/(-RC\d+|-dev|dev|-hotfix)$/, '') as int
+            if (v1Part != v2Part) {
+                return v1Part <=> v2Part
+            }
+        }
+
+        v1Parts.size() <=> v2Parts.size()
+    }
+
+    /**
+     * Checks if the current version is greater than the provided version
+     * @param otherVersion The version to compare against
+     * @return true if current version is greater, false otherwise
+     */
+    boolean isVersionGreaterThan(String otherVersion) {
+        def version = getVersion()
+        if (!version || !otherVersion) return false
+
+        // Strip any suffixes for comparison
+        def v1 = version.replaceAll(/(-RC\d+|-dev|dev|-hotfix)$/, '')
+        def v2 = otherVersion.replaceAll(/(-RC\d+|-dev|dev|-hotfix)$/, '')
+
+        compareVersions(v1, v2) > 0
+    }
+
+    /**
+     * Checks if the current version is compatible with the provided version
+     * @param otherVersion The version to check compatibility with
+     * @return true if compatible, false otherwise
+     */
+    boolean isVersionCompatibleWith(String otherVersion) {
+        def version = getVersion()
+        if (!version || !otherVersion) return false
+
+        // Development versions are always compatible
+        if (isDevelopmentVersion()) return true
+
+        // For release candidates and hotfixes, compare base versions
+        def v1 = version.replaceAll(/(-RC\d+|-dev|dev|-hotfix)$/, '')
+        def v2 = otherVersion.replaceAll(/(-RC\d+|-dev|dev|-hotfix)$/, '')
+
+        // Version should be greater than or equal to the other version
+        compareVersions(v1, v2) >= 0
+    }


All of these version-related methods seem to be assuming/enforcing a very specific versioning scheme on pipelines. I don't think Nextflow can make these kind of assumptions/expectations since any organisation could be writing their own Nextflow pipelines and might use a different versioning scheme which is completely incompatible.

It doesn't look like any of these methods are used in this PR though, so I think they can just be removed.

tom-seqera · 2025-01-21T09:25:32Z

modules/nextflow/src/main/groovy/nextflow/scm/AssetManager.groovy

@@ -937,9 +937,18 @@ class AssetManager {
        assert localPath

        def current = getCurrentRevision()
-        if( current != defaultBranch ) {
+        def defaultRev = getManifest().getDefaultRevision()


This shouldn't be asking the manifest directly; it needs to use the defaultBranch logic in AssetManager to handle cases where no value is specified in the manifest.

tom-seqera · 2025-01-21T09:31:27Z

modules/nextflow/src/main/groovy/nextflow/scm/AssetManager.groovy

+                Ref head = git.getRepository().findRef(Constants.HEAD);
+
+                // try to resolve the the current object id to a tag name
+                Map<ObjectId, String> names = git.nameRev().addPrefix( "refs/tags/" ).add(head.objectId).call()
+                def tag = names.get( head.objectId ) ?: head.objectId.name()
+                if( current != tag ) {


I'm not quite sure I understand this change.

It looks like the new logic boils down to "don't throw the error if the current revision is a tag (any tag)". Is that the intention?

tom-seqera · 2025-01-21T09:37:42Z

modules/nextflow/src/test/groovy/nextflow/scm/AssetManagerTest.groovy

@@ -602,7 +547,7 @@ class AssetManagerTest extends Specification {
    }

    @Requires({System.getenv('NXF_GITHUB_ACCESS_TOKEN')})
-    def 'should identify default branch when downloading repo'() {
+    def "should use a default tag"() {


This test name ('should identify default branch when downloading repo') was correct and shouldn't be changed. It tests that if the repo manifest doesn't explicitly specify a default branch (or revision) then Nextflow can correctly identify the default branch configured at the git level. In the case of the 'socks' repo, this is 'main'.

tom-seqera · 2025-01-21T09:39:48Z

modules/nextflow/src/test/groovy/nextflow/scm/AssetManagerTest.groovy

+        holder.manifest.getDefaultBranch() == 'master'
+    }
+
+    def "should use version as defaultRevision when available"() {


As mentioned, I'm not sure 'version' should be involved in determining the default revision/branch.

tom-seqera · 2025-01-21T09:48:58Z

modules/nextflow/src/test/groovy/nextflow/scm/AssetManagerTest.groovy

@@ -623,32 +568,470 @@ class AssetManagerTest extends Specification {
        noExceptionThrown()
    }

-    @Requires({System.getenv('NXF_GITHUB_ACCESS_TOKEN')})
-    def 'can filter remote branches'() {


This test ('can filter remote branches') should not be removed.

tom-seqera · 2025-01-21T09:53:42Z

modules/nextflow/src/test/groovy/nextflow/scm/AssetManagerTest.groovy

-    def testList() {
-
-        given:
+    def "should list available pipeline assets"() {


While I agree with efforts to improve the naming and clarity of the tests, doing so alongside changes to behaviour introduces a lot of noise. I'd suggest splitting such changes into a separate PR which improves the tests without modifying any behaviour.

I think this is a great idea. Let me do that first. This PR got convoluted because I wanted to make sure any changes I made were going to be backwards compatible 😄

bentsherman added the software/git label Dec 13, 2024

edmundmiller and others added 10 commits December 13, 2024 10:59

test: Should read a default tag from the manifest file

bc52d2b

First attempt at fixing tag

3fb101c

test(nextflow-io#4427): Write some specs

cc8350e

chore: Add logic explanation

9d9f19b

test: Add some more edgecases

8128830

fix: Handle release candidate versions

d1ebdcc

edmundmiller force-pushed the default-revision branch from 2da6757 to d1ebdcc Compare December 18, 2024 13:37

pditommaso force-pushed the master branch from 1f834a0 to 6454605 Compare January 20, 2025 15:13

tom-seqera reviewed Jan 21, 2025

View reviewed changes

edmundmiller mentioned this pull request Jan 24, 2025

Improve git tests #5711

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default revision #5610

Default revision #5610

edmundmiller commented Dec 13, 2024

netlify bot commented Dec 13, 2024 •

edited

Loading

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025

tom-seqera Jan 21, 2025 •

edited

Loading

edmundmiller Jan 24, 2025

Default revision #5610

Are you sure you want to change the base?

Default revision #5610

Conversation

edmundmiller commented Dec 13, 2024

netlify bot commented Dec 13, 2024 • edited Loading

❌ Deploy Preview for nextflow-docs-staging failed. Why did it fail? →

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-seqera Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Dec 13, 2024 •

edited

Loading

tom-seqera Jan 21, 2025 •

edited

Loading