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.

Sign up for GitHub

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

Jump to bottom

work on using files assets #3254

Merged
merged 1 commit into from
Dec 18, 2017
Merged

work on using files assets #3254

merged 1 commit into from
Dec 18, 2017

Conversation

chrislovecnm
Copy link
Contributor

@chrislovecnm chrislovecnm commented Aug 22, 2017 •
edited
Loading

Basic MVP for file assests.

  • using file assest builder
  • able to upload files
  • using URL structs instead of strings everywhere

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2017
@chrislovecnm
Copy link
Contributor Author

@justinsb we have an interesting chicken and egg problem. We should be pulling sha values from the staged sha1 files in the file registry, for assest like kubectl. Before a file assest is uploaded the sha1 file does not exist. The use case is where kops is running in an airgapped environment and not running the assests phase. Any ideas on how to handle this properly? Currently I am only using the source sha1 file, and I do not think this will work. Should I try the file registry and then try the source sha location?

@justinsb
Copy link
Member

I don't have a great answer for that particular scenario of yours. I'd say kops can easily copy the assets to a mirror location. Maybe then for the actual installation, if you're going to say kops can't read from the source locations, you pretend that the source location is the actual location. I don't know whether that means that you actually install the cluster from the mirror, or maybe you just configure the asset store to read hashes from the target instead of the source. I think the latter but I haven't tried it...

@KashifSaadat KashifSaadat mentioned this pull request Aug 23, 2017
Closed
@justinsb
Copy link
Member

So what I suggest @chrislovecnm is that you start with the simple case, where the source is available, and then we can add the behaviour for where the source is not available separately.

@k8s-github-robot k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2017
@justinsb justinsb added this to the backlog milestone Sep 25, 2017
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 13, 2017
@chrislovecnm chrislovecnm added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2017
@chrislovecnm chrislovecnm force-pushed the file-assets branch 4 times, most recently from 530f6b0 to 275a732 Compare October 18, 2017 17:48
krzyzacy
krzyzacy reviewed Oct 18, 2017
View reviewed changes
upup/pkg/fi/cloudup/urls.go Outdated
b = kopsUrl
}

fileUrl, hash, err := assetBuilder.RemapFileAndSHA(b+file, b+file+".sha1")
Copy link
Member

Choose a reason for hiding this comment

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

I think you probably want to use filepath.Join here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not certain it is there, but yes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Little more complicated to do a url ...

	baseUrl, err := url.Parse(baseUrlString)
        baseUrl.Path = path.Join(u.baseUrl, file)
        s := u.String()

@k8s-ci-robot k8s-ci-robot 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 Oct 19, 2017
@chrislovecnm chrislovecnm force-pushed the file-assets branch 2 times, most recently from 3de15b2 to 8d68702 Compare October 20, 2017 23:57
@k8s-github-robot
Copy link

@chrislovecnm PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2017
@chrislovecnm
Copy link
Contributor Author

/test pull-kops-e2e-kubernetes-aws

@k8s-github-robot
Copy link

@chrislovecnm PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2017
@chrislovecnm chrislovecnm force-pushed the file-assets branch 4 times, most recently from b7d9de8 to 37b3f30 Compare December 14, 2017 08:42
@chrislovecnm
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2017
@chrislovecnm chrislovecnm force-pushed the file-assets branch 4 times, most recently from e9362a7 to 8eb2ce3 Compare December 15, 2017 19:33
@chrislovecnm chrislovecnm removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-changes labels Dec 15, 2017
@chrislovecnm
Enabling the file assets
7057aaf 
File assets and the SHA files are uploaded to the new location. Files
when are users uses s3 are upload public read only. The copyfile task
uses only the existing SHA value.

This PR include major refactoring of the use of URLs.  Strings are no
longer categnated, but converted into a URL struct and path.Join is
utlilized.

A new values.go file is included so that we can start refactoring more
code out of the "fi" package.

A
@chrislovecnm chrislovecnm modified the milestones: backlog, 1.9 Dec 17, 2017
@justinsb
Copy link
Member

Some small things but I'll send PRs rather than continuing the cycle.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit dd49d69 into kubernetes:master Dec 18, 2017
This was referenced Dec 18, 2017
Closed
Closed
@KashifSaadat KashifSaadat mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@krzyzacy krzyzacy krzyzacy left review comments

Assignees

@justinsb justinsb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@chrislovecnm @justinsb @k8s-github-robot @k8s-ci-robot @krzyzacy