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

Allow uploading when config_manifest is not specified in the google artifact registry #115

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

linshokaku
Copy link
Contributor

#112

The issue causing errors in Google Artifact Registry when no config_manifest was specified has been resolved.

I could not reproduce this problem with the test method shown in the Developer's Guide, but I have confirmed that it is resolved in my own environment.

Signed-off-by: Linsho Kaku <linsho@preferred.jp>
Signed-off-by: Linsho Kaku <linsho@preferred.jp>
@linshokaku linshokaku force-pushed the no-manifest-config-push branch from 15363cb to 74a829c Compare November 13, 2023 06:51
Signed-off-by: Linsho Kaku <linsho@preferred.jp>
oras/provider.py Outdated
config_file = os.path.join(tmp, "config.json")
with open(config_file, "w") as f:
f.write("{}")
response = self.upload_blob(config_file, container, conf)
Copy link
Contributor

Choose a reason for hiding this comment

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

This response block happens either way, so why not just have the "if none do x" and then remove the else, and no matter what call response = xxxx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no file is specified, a temporary file must be created before sending, and then the resources provided to create the temporary file must be released.

Therefore, if no file is specified, it is necessary to execute upload_blob() in the scope under with TemporaryDirectory() as tmp:.

If a single call to upload_blos code is desired, the code would be such that it would require execution of TemporaryDirectory() even if the file exists.

with TemporaryDirectory() as tmp:
    if config_file is None:
        config_file = os.path.join(tmp, "config.json")
        with open(config_file, "w") as f:
            f.write("{}")
    response = self.upload_blob(config_file, container, conf)

Is this the correct modification?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "with" context is adding the extra else here - and I see your point about needing to clean up after (and it not working with the context). Would it be too simple to use the already provided functions

def get_tmpdir(
to create a directory or file, and then just clean it up after? And remove the redundant logic using the with context? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed using oras.utils.fileio.get_tmpfile() .

Signed-off-by: Linsho Kaku <linsho@preferred.jp>
oras/provider.py Outdated
@@ -747,7 +747,12 @@ def push(self, *args, **kwargs) -> requests.Response:

# Config is just another layer blob!
logger.debug(f"Preparing config {conf}")
if config_file is None:
config_file = oras.utils.get_tmpfile(suffix=".json")
with open(config_file, "w") as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use oras.utils.write_file here:

def write_file(
should just need to do:

oras.utils.write_file(config_file, "{}")

Copy link
Contributor

Choose a reason for hiding this comment

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

And I guess we are OK here not cleaning up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Fixing the use of write_file
  • Delete temporary files

Two corrections were made.

Signed-off-by: Linsho Kaku <linsho@preferred.jp>
@vsoch vsoch merged commit b6196ca into oras-project:main Nov 16, 2023
5 checks passed
@vsoch
Copy link
Contributor

vsoch commented Nov 16, 2023

Thanks!

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.

2 participants