-
Notifications
You must be signed in to change notification settings - Fork 88
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
Using an OCI compliant image registry instead of S3 for storage #523
Conversation
aed7988
to
b0375d5
Compare
# - name: STORAGE_BASEURI | ||
# value: "docker://kotsadm-storage-registry:5000" | ||
# - name: STORAGE_BASEURI_PLAINHTTP | ||
# value: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these stay commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was torn on this one. Enabling this makes the environment use the new OCI storage. But there's no migration, so if these are present, it will break envs.
Any thoughts on how you'd like to see this rolled out as an alpha feature to the skaffold environment?
@@ -78,6 +80,7 @@ func Start() { | |||
r.Path("/api/v1/upload").Methods("PUT").HandlerFunc(handlers.UploadExistingApp) | |||
r.Path("/api/v1/download").Methods("GET").HandlerFunc(handlers.DownloadApp) | |||
r.Path("/api/v1/app/{appSlug}/sequence/{sequence}/renderedcontents").Methods("OPTIONS", "GET").HandlerFunc(handlers.GetAppRenderedContents) | |||
r.Path("/api/v1/app/{appSlug}/sequence/{sequence}/contents").Methods("OPTIONS", "GET").HandlerFunc(handlers.GetAppContents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this an entirely new route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It used to be a GQL route. This is used on the ViewFiles page to render the application contents. I didn't want to put the OCI get/put code in Typescript, so I ported this endpoint to Go /Rest.
kotsadm/pkg/handlers/contents.go
Outdated
return | ||
} | ||
|
||
JSON(w, 200, map[string]interface{}{"files": archiveFiles}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make this into a better defined type?
at least map[string]map[string][]byte
Success: false, | ||
} | ||
|
||
sess, err := session.Parse(r.Header.Get("Authorization")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wish we could do this in a middleware like with gin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We should write a design proposal up for this change
kotsadm/pkg/handlers/troubleshoot.go
Outdated
io.Copy(w, f) | ||
} | ||
|
||
func UploadTroubleshootBundle(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we call this "UploadSupportBundle", like the other functions here?
"github.com/replicatedhq/kots/kotsadm/pkg/supportbundle/types" | ||
"github.com/segmentio/ksuid" | ||
) | ||
|
||
func SetBundleAnalysis(id string, insights []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this never called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to analysis.go. this file was big, splitting it up since i added a lot more to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 the downsides of marking files as 'read' as you go is that you can't cmd+f them
This is an experimental feature to fix #520. It also adds docker/distribution to the local dev env (leaving minio also). This is not enabled in dev by default, so no dev env migration is necessary.
This will not impact deployed versions because this feature is not enabled by default.
Some quality of life todos before merging:
Some new things: