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

Reuse timezone code from containers/common #21332

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Reuse timezone code from containers/common #21332

merged 2 commits into from
Feb 8, 2024

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jan 22, 2024 •
edited
Loading

Replaces: #21077

[NO NEW TESTS NEEDED] Existing tests should handle this.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jan 22, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jan 22, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@mheon
Copy link
Member

mheon commented Jan 22, 2024

LGTM but you have build failures

@TomSweeneyRedHat
Copy link
Member

LGTM
once the tests get hip

@rhatdan rhatdan force-pushed the timezone branch 2 times, most recently from d20f529 to 69060e4 Compare January 23, 2024 16:30
@Luap99 Luap99 mentioned this pull request Jan 24, 2024
Merged
@rhatdan rhatdan added the bloat_approved Approve a PR in which binary file size grows by over 50k label Jan 24, 2024
@rhatdan
Copy link
Member Author

rhatdan commented Jan 24, 2024

@dfr PTAL Won't work on freebsd?

@rhatdan rhatdan changed the title Reuse timezone code from containers/common [WIP] Reuse timezone code from containers/common Jan 24, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2024
@dfr
Copy link
Contributor

dfr commented Jan 25, 2024

@dfr PTAL Won't work on freebsd?

I think this should work on FreeBSD - it uses /etc/localtime in the same way as Linux.

@dfr
Copy link
Contributor

dfr commented Jan 25, 2024

I tested this on FreeBSD and it doesn't work correctly - I think the /etc/localtime symlink is wrong:

$ sudo ./bin/podman run -ti --rm --tz=America/New_York localhost/freebsd14.0-minimal sh
ERRO[0000] unexpected OS version: 14.0-RELEASE-p4
# date
Thu Jan 25 15:50:25 UTC 2024
# TZ=America/New_York date
Thu Jan 25 10:50:33 EST 2024
# ls -l /etc/localtime
lrwxr-xr-x  1 root wheel 137 Jan 25 15:49 /etc/localtime -> /var/db/containers/storage/zfs/graph/3d995d201f15d80cd00eb481c480da43afd7d7c5bc0e77fce240252dfaddcfef/usr/share/zoneinfo/America/New_York
#

(ignore the 'unexpected OS' error - l think thats a bug in one of my PRs from last year)

@rhatdan
Copy link
Member Author

rhatdan commented Jan 27, 2024

@umohnani8 Looks like your filter changes are breaking when containers/common tries to get vendored in.

@rhatdan rhatdan force-pushed the timezone branch 3 times, most recently from de285ac to a8512ea Compare January 28, 2024 19:59
@dfr
Copy link
Contributor

dfr commented Jan 29, 2024

I tested this on FreeBSD and it doesn't work correctly - I think the /etc/localtime symlink is wrong:

Does the symlink option work correctly on Linux? For me, it includes the full host path to the container's usr/share/zoneinfo directory. Perhaps we should always copy since these files are tiny?

@rhatdan
Copy link
Member Author

rhatdan commented Jan 29, 2024

@dfr working on another fix.

@dfr
Copy link
Contributor

dfr commented Jan 29, 2024

The latest iteration with the relative symlink works fine on FreeBSD

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2024
@rhatdan rhatdan changed the title [WIP] Reuse timezone code from containers/common Reuse timezone code from containers/common Feb 1, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2024
@rhatdan rhatdan force-pushed the timezone branch 2 times, most recently from 1666f23 to f1fc48e Compare February 2, 2024 01:07
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2024
@rhatdan
Vendor in containers/common
85904e0 
Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Reuse timezone code from containers/common
fcae702 
Replaces: containers#21077

[NO NEW TESTS NEEDED] Existing tests should handle this.

Signed-off-by: Sohan Kunkerkar <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan rhatdan added 5.0 lgtm Indicates that a PR is ready to be merged. labels Feb 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 5e081e4 into containers:main Feb 8, 2024
88 of 89 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 9, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
5.0 approved Indicates a PR has been approved by an approver from all required OWNERS files. bloat_approved Approve a PR in which binary file size grows by over 50k lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rhatdan @mheon @TomSweeneyRedHat @dfr @openshift-merge-robot