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

Wget access method #630

Merged
merged 23 commits into from
Feb 26, 2024

Conversation

fabianburth
Copy link
Contributor

Description

This PR implements a new access method type called wget. This access types allows to reference and access resources located on a http server.

@fabianburth fabianburth marked this pull request as draft January 17, 2024 13:51
@fabianburth fabianburth marked this pull request as ready for review January 19, 2024 07:54
@@ -0,0 +1,24 @@
// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Open Component Model contributors.
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
// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Open Component Model contributors.
// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and Open Component Model contributors.

Where is this text coming from? Can we change it centrally?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be generated by the IDE (if configured). In Goland you should use the variable $today.year instead of a fixed year.


var blob cpi.BlobAccess
if resp.ContentLength < 0 || resp.ContentLength > CACHE_CONTENT_THRESHOLD {
log.Debug("download to file because content length is", "unknown or greater than", CACHE_CONTENT_THRESHOLD)
Copy link
Member

Choose a reason for hiding this comment

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

Why is the log message splitted like this? Would it be too long?

Suggested change
log.Debug("download to file because content length is", "unknown or greater than", CACHE_CONTENT_THRESHOLD)
log.Debug("download to file because content length is unknown or greater than", CACHE_CONTENT_THRESHOLD)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The signature of the log function is Debug(msg string, keypairs ...interface{}). This quite weird and I just thought I'd provide a key value pair.

Copy link
Contributor

Choose a reason for hiding this comment

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

The message should be download to file because content length is unknown or greater than {{threshold}} The key/value pair then is "threshold", CACHE_CONTENT_THRESHOLD.

You could omit the curly brackets. They are used by the human log message formatter to provide human-readable log output by substituting known values (and omitting them from the shown value list).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted.

hilmarf
hilmarf previously approved these changes Jan 23, 2024
Copy link
Member

@hilmarf hilmarf left a comment

Choose a reason for hiding this comment

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

only a few questions and comments...
at one place there is a copy & paste issue: search for 'Helm Chart' ;-)

hilmarf
hilmarf previously approved these changes Jan 24, 2024
hilmarf
hilmarf previously approved these changes Jan 24, 2024
@@ -0,0 +1,24 @@
// SPDX-FileCopyrightText: 2022 SAP SE or an SAP affiliate company and Open Component Model contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be generated by the IDE (if configured). In Goland you should use the variable $today.year instead of a fixed year.

cmds/ocm/commands/ocmcmds/common/inputs/types/wget/spec.go Outdated Show resolved Hide resolved
pkg/contexts/credentials/utils.go Show resolved Hide resolved
pkg/contexts/ocm/accessmethods/wget/method.go Outdated Show resolved Hide resolved
@fabianburth fabianburth force-pushed the wget-access-method branch 2 times, most recently from aff20c0 to 4b9c6c4 Compare February 14, 2024 08:48
.reuse/dep5 Show resolved Hide resolved
@mandelsoft
Copy link
Contributor

There are still lint errors

Copy link
Contributor

@mandelsoft mandelsoft left a comment

Choose a reason for hiding this comment

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

There are still lint errors.

@mandelsoft mandelsoft merged commit 2c12eb2 into open-component-model:main Feb 26, 2024
13 of 14 checks passed
@fabianburth fabianburth deleted the wget-access-method branch February 26, 2024 08:28
fabianburth added a commit to open-component-model/ocm-spec that referenced this pull request Apr 22, 2024
The ocm tooling introduced the wget access method
[here](open-component-model/ocm#630). This adds
documentation to the corresponding spec section.
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.

3 participants