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

Refactor copy packages command to support regional ECR #6747

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

d8660091
Copy link
Member

@d8660091 d8660091 commented Sep 29, 2023

Issue #, if available:

Description of changes:

  • no more dependency on running docker containers
  • support for regional curated packages

Testing (if applicable):

  • Manually tested copy packages from regional ECR to local registry
  • Manually tested copy packages from global ECRs to local registry
  • Manually tested copy packages from global ECRs to regional ECR

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 29, 2023
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 114 lines in your changes are missing coverage. Please review.

Comparison is base (c6ff83b) 75.66% compared to head (c45e857) 71.68%.
Report is 6 commits behind head on main.

❗ Current head c45e857 differs from pull request most recent head c2b44bf. Consider uploading reports for the commit c2b44bf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6747      +/-   ##
==========================================
- Coverage   75.66%   71.68%   -3.99%     
==========================================
  Files         475      531      +56     
  Lines       38404    41051    +2647     
==========================================
+ Hits        29059    29427     +368     
- Misses       7733     9978    +2245     
- Partials     1612     1646      +34     
Files Coverage Δ
cmd/eksctl-anywhere/cmd/copypackages.go 22.09% <18.57%> (ø)

... and 56 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eks-distro-bot eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 29, 2023
@d8660091 d8660091 force-pushed the refactor-cp-package branch 2 times, most recently from 9c00867 to c45e857 Compare September 29, 2023 18:35
@eks-distro-bot eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 29, 2023
@d8660091
Copy link
Member Author

/retest

Copy link
Member

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

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

I left a few comments.

--dst-cert ${REGISTRY_MIRROR_CERT_PATH} \
${REGISTRY_MIRROR_URL}
```
If your EKSA cluster is running in an airgapped environment and you set up a local registry mirror, you can copy curated packages from Amazon ECR to your local registry mirror with the following command.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If your EKSA cluster is running in an airgapped environment and you set up a local registry mirror, you can copy curated packages from Amazon ECR to your local registry mirror with the following command.
If your EKS Anywhere cluster is running in an airgapped environment and you set up a local registry mirror, you can copy curated packages from Amazon ECR to your local registry mirror with the following command.

```
If your EKSA cluster is running in an airgapped environment and you set up a local registry mirror, you can copy curated packages from Amazon ECR to your local registry mirror with the following command.

`$KUBEVERSION` must equals to the `spec.kubernetesVersion` of your EKS Anywhere cluster specification.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`$KUBEVERSION` must equals to the `spec.kubernetesVersion` of your EKS Anywhere cluster specification.
`$KUBEVERSION` be must equal to the `spec.kubernetesVersion` of your EKS Anywhere cluster specification.

@@ -26,5 +26,5 @@ Copy EKS Anywhere resources and artifacts
### SEE ALSO

* [anywhere](../anywhere/) - Amazon EKS Anywhere
* [anywhere copy packages](../anywhere_copy_packages/) - Copy curated package images and charts from a source to a destination
* [anywhere copy packages](../anywhere_copy_packages/) - Copy curated package images and charts from source regisries to a destination registry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* [anywhere copy packages](../anywhere_copy_packages/) - Copy curated package images and charts from source regisries to a destination registry
* [anywhere copy packages](../anywhere_copy_packages/) - Copy curated package images and charts from source registries to a destination registry

@@ -5,11 +5,11 @@ linkTitle: "anywhere copy packages"

## anywhere copy packages

Copy curated package images and charts from a source to a destination
Copy curated package images and charts from source regisries to a destination registry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copy curated package images and charts from source regisries to a destination registry
Copy curated package images and charts from source registries to a destination registry

-h, --help help for packages
--insecure Skip TLS verification while copying images and charts
--src-cert string TLS certificate for source registry
--dry-run Dry run will not really copy the artifacts, but shows what artifacts would be copied
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--dry-run Dry run will not really copy the artifacts, but shows what artifacts would be copied
--dry-run Dry run will show what artifacts would be copied, but not actually copy them

--src-cert string TLS certificate for source registry
--dry-run Dry run will not really copy the artifacts, but shows what artifacts would be copied
--dst-insecure Skip TLS verification against the destination registry
--dst-plain-http Whether to use plain http for destination registry
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--dst-plain-http Whether to use plain http for destination registry
--dst-plain-http Whether or not to use plain http for destination registry

Is the alternative https?

--dst-insecure Skip TLS verification against the destination registry
--dst-plain-http Whether to use plain http for destination registry
-h, --help help for packages
--kube-version string The kube version of the package bundle to copy
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--kube-version string The kube version of the package bundle to copy
--kube-version string The Kubernetes version of the package bundle to copy

Copy link
Member

@jonathanmeier5 jonathanmeier5 left a comment

Choose a reason for hiding this comment

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

Generally lgtm, I think it's fine to refactor for testing later.

}

// Since package bundle doesn't contain tags, we get tags from chart values.
func getTagsFromCharts(ctx context.Context, helm *executables.Helm, charts []registry.Artifact) (map[string]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this used any more? Should we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, removed

@eks-distro-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@d8660091
Copy link
Member Author

d8660091 commented Oct 3, 2023

/approve

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: d8660091

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@eks-distro-bot eks-distro-bot merged commit 45aaee6 into aws:main Oct 3, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/docs Documentation documentation lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants