Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-55017

Allow advanced configuration for other s3 providers

    XMLWordPrintable

    Details

    • Similar Issues:
    • Released As:
      1.8

      Description

      Currently the plugin hard codes the URL template here: https://github.com/jenkinsci/artifact-manager-s3-plugin/blob/master/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3BlobStore.java#L164

      I would love to be able to customize this via some sort of advanced config. There are a number of providers that provide S3 compatible API. Would love to be able to use them.

        Attachments

          Activity

          gena01 Gennady Feldman created issue -
          Hide
          christapley Chris Tapley added a comment -

          I believe I have a working implementation of this in a fork https://github.com/christapley/artifact-manager-s3-plugin.

          Jesse Glick, would you accept a pr for this? Are there any special requirements to get this accepted?

          Show
          christapley Chris Tapley added a comment - I believe I have a working implementation of this in a fork https://github.com/christapley/artifact-manager-s3-plugin . Jesse Glick , would you accept a pr for this? Are there any special requirements to get this accepted?
          Hide
          jglick Jesse Glick added a comment -

          Great! In general, the main things that help a PR get merged:

          • clearly stated goal
          • minimal diff (no lines not directly contributing to goal)
          • code structure making it obvious that existing scenarios would not regress
          • convincing test coverage

          Test coverage in this plugin is a hard problem, unfortunately. I run tests manually against S3 before release, and would run them against a tricky-looking PR before merge. (There used to be a private CI builder for this plugin with an AWS account, but that was shut down since it is not being actively developed: my employer is not currently prioritizing this over other things.)

          On the other hand, for this particular change there is an OSS provider which claims S3 compatibility—MinIO—so there may be a way to offer self-contained test coverage: use either Docker Fixtures or TestContainers to include a test suite with a Docker fixture for MinIO and delegate to ArtifactManagerTest.artifactStashAndDelete; see JCloudsArtifactManagerTest for the live AWS version.

          (I am assuming that S3BlobStore.toExternalURL actually works in MinIO. Various documentation suggests that it supports presigned URLs, but I have never tried it. See the plugin README for discussion of how presigned URLs are critical to the security model.)

          Show
          jglick Jesse Glick added a comment - Great! In general, the main things that help a PR get merged: clearly stated goal minimal diff (no lines not directly contributing to goal) code structure making it obvious that existing scenarios would not regress convincing test coverage Test coverage in this plugin is a hard problem, unfortunately. I run tests manually against S3 before release, and would run them against a tricky-looking PR before merge. (There used to be a private CI builder for this plugin with an AWS account, but that was shut down since it is not being actively developed: my employer is not currently prioritizing this over other things.) On the other hand, for this particular change there is an OSS provider which claims S3 compatibility— MinIO —so there may be a way to offer self-contained test coverage: use either Docker Fixtures or TestContainers to include a test suite with a Docker fixture for MinIO and delegate to ArtifactManagerTest.artifactStashAndDelete ; see JCloudsArtifactManagerTest for the live AWS version. (I am assuming that S3BlobStore.toExternalURL actually works in MinIO. Various documentation suggests that it supports presigned URLs, but I have never tried it. See the plugin README for discussion of how presigned URLs are critical to the security model.)
          Hide
          christapley Chris Tapley added a comment -

          I have been adding tests as I go, but I will also look at Docker Fixtures or TestContainers for better test coverage.

          I have been testing with AWS and minio. Minio does indeed appear to support presigned URLs.

          Show
          christapley Chris Tapley added a comment - I have been adding tests as I go, but I will also look at Docker Fixtures or TestContainers for better test coverage. I have been testing with AWS and minio. Minio does indeed appear to support presigned URLs.
          Hide
          christapley Chris Tapley added a comment -

          Hi Jesse Glick, I've submitted the PR. Please let me know what you think.

          Show
          christapley Chris Tapley added a comment - Hi Jesse Glick , I've submitted the PR. Please let me know what you think.
          jglick Jesse Glick made changes -
          Field Original Value New Value
          Assignee Chris Tapley [ christapley ]
          jglick Jesse Glick made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          jglick Jesse Glick made changes -
          Status In Progress [ 3 ] In Review [ 10005 ]
          jglick Jesse Glick made changes -
          Remote Link This issue links to "artifact-manager-s3 #96 (Web Link)" [ 24237 ]
          jglick Jesse Glick made changes -
          Remote Link This issue links to "artifact-manager-s3 #109 (Web Link)" [ 24243 ]
          jglick Jesse Glick made changes -
          Assignee Chris Tapley [ christapley ] Jesse Glick [ jglick ]
          jglick Jesse Glick made changes -
          Status In Review [ 10005 ] In Progress [ 3 ]
          jglick Jesse Glick made changes -
          Status In Progress [ 3 ] In Review [ 10005 ]
          jglick Jesse Glick made changes -
          Released As 1.8
          Resolution Fixed [ 1 ]
          Status In Review [ 10005 ] Fixed but Unreleased [ 10203 ]
          jglick Jesse Glick made changes -
          Status Fixed but Unreleased [ 10203 ] Resolved [ 5 ]

            People

            Assignee:
            jglick Jesse Glick
            Reporter:
            gena01 Gennady Feldman
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: