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

Add support for overriding Created and Last-Modified dates on deploy #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

capyvara
Copy link

@capyvara capyvara commented Mar 3, 2022

On migration/mirroring use cases, it's useful to be able to change the Created and Last Modified dates of the deployed artifacts, this PR allows to pass up an optional datetime override for both fields.

This is done by adding the X-Artifactory-Created and X-Artifactory-Last-Modified fields

@beliaev-maksim
Copy link
Member

can you please add link to JFrog the documentation in PR description
add info to the changelog
and fix pre-commit issues

@@ -245,6 +245,12 @@ def sha256sum(filename):
return sha256.hexdigest()


def datetime_xheader(datetime):
Copy link
Member

Choose a reason for hiding this comment

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

The function datetime_xheader doesn't work with headers, it's a serializer.
if isinstance(datetime, datetime.datetime): doesn't it lead to a bug? because datetime is not a module anymore, it's a function argument datetime_xheader(datetime)

I think if we return int, not str - it works in headers too.

Could you add a test on this?

Copy link
Author

Choose a reason for hiding this comment

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

I must confess that I've not checked it, you're right the naming clash will likely mess this up, do you think it's necessary to allow the string version? I only used a datetime conversion at my code.

This is the code I've done as a hack, in which I've tried and the header works in setting up the created/last-modified date on the server:

def datetime_header(datetime):
    return str(int(round(1000*datetime.timestamp())))

# Custom version to allow change of Created and Last-Modified
def deploy_file(target, file_name, created, last_modified, parameters):
    md5 = md5sum(file_name)
    sha1 = sha1sum(file_name)
    sha256 = sha256sum(file_name)
    target = target / pathlib.Path(file_name).name

    with open(file_name, "rb") as fobj:
        url = str(target)

        matrix_parameters = (
            f";{encode_matrix_parameters(parameters)}" if parameters else None
        )
        headers = {}

        headers["X-Checksum-Md5"] = md5
        headers["X-Checksum-Sha1"] = sha1
        headers["X-Checksum-Sha256"] = sha256
        headers["X-Artifactory-Created"] = datetime_header(created)
        headers["X-Artifactory-Last-Modified"] = datetime_header(last_modified)

        target._accessor.rest_put_stream(
            url,
            fobj,
            headers=headers,
            session=target.session,
            verify=target.verify,
            cert=target.cert,
            timeout=target.timeout,
            matrix_parameters=matrix_parameters,
        )

Copy link
Member

@allburov allburov left a comment

Choose a reason for hiding this comment

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

Could you add a test on this?

@capyvara
Copy link
Author

capyvara commented Mar 6, 2022

@beliaev-maksim I don't think this is documented, I stole it from their nexus migration code: https://github.com/jfrog/nexus2artifactory/blob/master/nex2art/core/Upload.py

Do you think it's a problem?

@allburov
Copy link
Member

allburov commented Mar 9, 2022

@capyvara @beliaev-maksim
May be we can add the hack somewhere in the FAQ section in the readme.md ?

I'm not sure that we need to have dedicated parameters for the case, people use it rarely, but we should document this especially because official Artifactory Docs doesn't have the headers.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants