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 epoch_int in date_time facts #73822

Merged
merged 4 commits into from
Apr 16, 2021
Merged

Conversation

aminvakil
Copy link
Contributor

@aminvakil aminvakil commented Mar 8, 2021

SUMMARY

Add epoch_int to setup facts which always returns epoch as an integer.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/facts/system/date_time.py

ADDITIONAL INFORMATION

Previously opened in #72503.

@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. labels Mar 8, 2021
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. small_patch labels Mar 8, 2021
@aminvakil
Copy link
Contributor Author

ready_for_review

@ansibot ansibot added the feature This issue/PR relates to a feature request. label Mar 8, 2021
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Mar 11, 2021
@mattclay mattclay added this to the 2.12 milestone Mar 11, 2021
@@ -49,6 +49,9 @@ def collect(self, module=None, collected_facts=None):
date_time_facts['epoch'] = now.strftime('%s')
if date_time_facts['epoch'] == '' or date_time_facts['epoch'][0] == '%':
date_time_facts['epoch'] = str(int(epoch_ts))
date_time_facts['epoch_int'] = str(int(now.strftime('%s')))
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining why this is different from the existing epoch value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR and ansible-collections/ansible.windows#123 were created because of this issue: #72479

Current epoch value returns float and string in some circumstances, epoch_int is guaranteed to be integer.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have been more specific. Can you add a comment to the code explaining the possibility of the current epoch being a float? That will help anyone in the future looking at the code, since it's otherwise not obvious why we need both epoch and epoch_int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

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 in d2c161d.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. This PR is assigned to the 2.12 milestone, which we'll be going through after creating the stable-2.11 branch (which will be done when 2.11 RC1 comes out).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing!

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 11, 2021
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 25, 2021
@s-hertel s-hertel merged commit 0c101f3 into ansible:devel Apr 16, 2021
@aminvakil aminvakil deleted the date_time_epoch_int branch April 16, 2021 15:05
@ansible ansible locked and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants