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

Fix Checksum logic in get_url #64626

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

hugocartwright
Copy link

@hugocartwright hugocartwright commented Nov 8, 2019

SUMMARY

An attempt at resolving issue #64016, more details at:

Fixes #64016

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

get_url.py

ADDITIONAL INFORMATION

@mscherer @pilou-


mscherer and others added 5 commits Nov 1, 2019
non root user, since it fail with permission error
Since httpbin.org/get do return the IP of origin, the sha256 may
change. By using the /html endpoint and a dynamically computed
checksum.
- <checksum_mismatch> is now <checksum_match>
@ansibot
Copy link
Contributor

ansibot commented Nov 8, 2019

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Nov 8, 2019
@Akasurde Akasurde changed the title Fix 64016 Fix Checksum logic in get_url Nov 9, 2019
@Akasurde Akasurde requested a review from sivel Nov 9, 2019
@jpmens
Copy link
Contributor

jpmens commented Nov 9, 2019

shipit

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Nov 9, 2019
sivel
sivel previously approved these changes Nov 11, 2019
Copy link
Member

@sivel sivel left a comment

edit: I was incorrect about passing tests, I was looking at the wrong CI tab.

@sivel sivel dismissed their stale review Nov 11, 2019

Invalid

@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 Nov 19, 2019
Copy link

@whs-dot-hk whs-dot-hk left a comment

Hi, this are my observations

@@ -529,37 +529,38 @@ def main():
module.fail_json(msg='The checksum format is invalid', **result)

if not dest_is_dir and os.path.exists(dest):
checksum_mismatch = False
# should start with checksum_match = False for more clarity
Copy link

@whs-dot-hk whs-dot-hk Nov 28, 2019

Choose a reason for hiding this comment

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

Comment checksum_match default should be True, not False

# because last_mod_time may be newer than on remote
if checksum_mismatch:
force = True
if (checksum and checksum_match) or not checksum:

Choose a reason for hiding this comment

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

Condition 1:

if (checksum and checksum_match) or not checksum:

Actually same as
Condition 2:

if checksum_match:

because

checksum checksum_match Condition 1 Condition 2
F always T T T
T T T T
T F F F

Copy link
Author

@hugocartwright hugocartwright Nov 29, 2019

Choose a reason for hiding this comment

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

Thanks for the 101 programming lesson @whs-dot-hk maybe @sivel and the team can review the specs and naming conventions on the get_url module together with @gtie to reduce confusion between contributors.

Choose a reason for hiding this comment

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

I think it is not the naming

This piece of code is the logic for whether re-download or not

Copy link
Author

@hugocartwright hugocartwright Nov 29, 2019

Choose a reason for hiding this comment

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

@whs-dot-hk I think you made a request to work on this issue earlier in the week, if you have a solution you are welcome to take over.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jan 9, 2020
@ansibot ansibot removed 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 Jan 9, 2020
@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 Jan 17, 2020
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 18, 2020
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Dec 5, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 5, 2020
@bcoca
Copy link
Member

bcoca commented Jul 20, 2022

waiting_on_contributor

@ansibot ansibot added the waiting_on_contributor This would be accepted but there are no plans to actively work on it. label Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. net_tools Net-tools category new_contributor This PR is the first contribution by a new community member. pre_azp This PR was last tested before migration to Azure Pipelines. support:core This issue/PR relates to code supported by the Ansible Engineering Team. waiting_on_contributor This would be accepted but there are no plans to actively work on it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants