Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support archive.none for OSS directory artifacts #6312

Merged
merged 4 commits into from
Jul 27, 2021

Conversation

jibuji
Copy link
Contributor

@jibuji jibuji commented Jul 8, 2021

to fix problem: #6293
I have used the following workflow to test, and the result is correct.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: test-archive-none
spec:
  entrypoint: oss-getter-and-uploader
  templates:
  - name: oss-getter-and-uploader
    inputs:
      artifacts:
      - name: oss-none-empty-dir
        path: "/mnt/input/none-empty"
        oss:
          key: "some/none-empty"
        archive:
          none: {}
      - name: oss-empty-dir
        path: "/mnt/input/empty-dir"
        oss:
          key: "some/emptydir"
        archive:
          none: {}
      - name: oss-file
        path: "/mnt/input/file"
        oss:
          key: "some/file"
        archive:
          none: {}
      - name: oss-3level
        path: "/mnt/input/3level"
        oss:
          key: "some/3level-dir"
        archive:
          none: {}
    container:
      image: 'alpine:latest'
      command: ['/bin/sh', '-c']
      args: ["cp -rf /mnt/input /mnt/output"]
      volumeMounts:
        - name: out
          mountPath: /mnt
    volumes:
      - name: out
        emptyDir: { }
    outputs:
      artifacts:
      - name: oss-none-empty-dir
        path: "/mnt/output/none-empty"
        oss:
          key: "some2/none-empty"
        archive:
          none: {}
      - name: oss-empty-dir
        path: "/mnt/output/empty-dir"
        oss:
          key: "some2/emptydir"
        archive:
          none: {}
      - name: oss-file
        path: "/mnt/output/file"
        oss:
          key: "some2/file"
        archive:
          none: {}
      - name: oss-3level
        path: "/mnt/output/3level"
        oss:
          key: "some2/3level-dir"
        archive:
          none: {}

Thanks to @terrytangyuan a lot. He helped me to rectify some code, but I failed to commit the PR because my mistake. So, I open this one and commits again.

Signed-off-by: jibuji <sunnypengfei@gmail.com>
Signed-off-by: jibuji <sunnypengfei@gmail.com>
@terrytangyuan
Copy link
Member

Original review feedback is in #6296

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. All feedback in previous review have been addressed.

@jibuji
Copy link
Contributor Author

jibuji commented Jul 9, 2021

Thanks. What should I do next? This is my first time to contribute.

@terrytangyuan
Copy link
Member

terrytangyuan commented Jul 9, 2021

Let's wait for one of the approvers to take another look for any additional feedback before merging.

USERS.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #6312 (aaa7c16) into master (c2360c4) will increase coverage by 1.10%.
The diff coverage is 39.21%.

❗ Current head aaa7c16 differs from pull request most recent head 9b2d7fc. Consider uploading reports for the commit 9b2d7fc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6312      +/-   ##
==========================================
+ Coverage   47.84%   48.95%   +1.10%     
==========================================
  Files         250      252       +2     
  Lines       15667    18044    +2377     
==========================================
+ Hits         7496     8833    +1337     
- Misses       7232     8243    +1011     
- Partials      939      968      +29     
Impacted Files Coverage Δ
config/config.go 31.57% <ø> (-39.26%) ⬇️
pkg/apis/workflow/v1alpha1/workflow_types.go 49.82% <ø> (+0.20%) ⬆️
server/auth/sso/sso.go 25.90% <0.00%> (-0.90%) ⬇️
util/kubeconfig/kubeconfig.go 25.19% <0.00%> (+3.76%) ⬆️
util/kubeconfig/roundtripper.go 0.00% <0.00%> (ø)
workflow/artifacts/oss/oss.go 3.92% <0.00%> (-1.69%) ⬇️
workflow/common/common.go 80.00% <ø> (+5.00%) ⬆️
workflow/metrics/metrics.go 76.11% <0.00%> (-4.47%) ⬇️
workflow/util/merge.go 61.53% <0.00%> (+2.44%) ⬆️
workflow/cron/operator.go 38.35% <50.00%> (+0.98%) ⬆️
... and 254 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2360c4...9b2d7fc. Read the comment docs.

Signed-off-by: jibuji <sunnypengfei@gmail.com>
@sarabala1979
Copy link
Member

@jibuji PR looks good. can you add a test?

@alexec alexec self-assigned this Jul 27, 2021
@alexec alexec merged commit 40b0824 into argoproj:master Jul 27, 2021
This was referenced Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants