Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-19916

Investigate URI inconsistencies in daf_butler LocationFactory

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: daf_butler
    • Labels:
      None
    • Story Points:
      2
    • Sprint:
      Arch 2019-05-20, Arch 2019-05-27, Arch 2019-06-03
    • Team:
      Architecture

      Description

      In a GitHub pull request Dino Bektesevic makes the case that URIs in the Location classes are not working properly. His fix is not quite right so take the PR and see if it's possible to fix things such that URIs with relative paths are properly handled.

        Attachments

          Activity

          Hide
          tjenness Tim Jenness added a comment -

          Andy Salnikov Would you be able to review this please? As I say on the PR, I've changed most of the API that was only used within Location and LocationFactory, such that it is now consistently using URIs for the datastore root, and relative paths for the location inside the datastore. This makes things much simpler. The API as used by butler did not change and all tests still pass. The Location classes now should be able to work fine for s3 URIs for example.

          Show
          tjenness Tim Jenness added a comment - Andy Salnikov Would you be able to review this please? As I say on the PR, I've changed most of the API that was only used within Location and LocationFactory, such that it is now consistently using URIs for the datastore root, and relative paths for the location inside the datastore. This makes things much simpler. The API as used by butler did not change and all tests still pass. The Location classes now should be able to work fine for s3 URIs for example.
          Hide
          salnikov Andy Salnikov added a comment -

          Sorry for delay, I'll look at it.

          Show
          salnikov Andy Salnikov added a comment - Sorry for delay, I'll look at it.
          Hide
          tjenness Tim Jenness added a comment -

          Hold off for a minute. I'm tweaking it a bit because after reviewing the S3Datastore pull request I realized that it would help a lot if I added an explicit ButlerURI class. Almost done with that and I'll let you know.

          Show
          tjenness Tim Jenness added a comment - Hold off for a minute. I'm tweaking it a bit because after reviewing the S3Datastore pull request I realized that it would help a lot if I added an explicit ButlerURI class. Almost done with that and I'll let you know.
          Hide
          tjenness Tim Jenness added a comment -

          Andy Salnikov sorry for the delay. I've added a ButlerURI class to this ticket. This will hopefully help Dino Bektesevic in his s3 updates. I've added tests for file paths, file: URIs, and normal s3 and https URIs.

          Show
          tjenness Tim Jenness added a comment - Andy Salnikov sorry for the delay. I've added a ButlerURI class to this ticket. This will hopefully help Dino Bektesevic in his s3 updates. I've added tests for file paths, file: URIs, and normal s3 and https URIs.
          Hide
          salnikov Andy Salnikov added a comment -

          Looks OK, few comments on PR.

          Show
          salnikov Andy Salnikov added a comment - Looks OK, few comments on PR.
          Hide
          tjenness Tim Jenness added a comment -

          Merged.

          Show
          tjenness Tim Jenness added a comment - Merged.

            People

            • Assignee:
              tjenness Tim Jenness
              Reporter:
              tjenness Tim Jenness
              Reviewers:
              Andy Salnikov
              Watchers:
              Andy Salnikov, Dino Bektesevic, Jim Bosch, Tim Jenness
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel