Skip to content

Commit

Permalink
change default value of quote_parameters to True (#422)
Browse files Browse the repository at this point in the history
  • Loading branch information
@briantist
briantist authored Dec 14, 2023
1 parent 3735e88 commit 2e92742
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 57 deletions.
24 changes: 3 additions & 21 deletions artifactory.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import sys
import urllib.parse
from itertools import islice
from warnings import warn

import dateutil.parser
import requests
Expand Down Expand Up @@ -1145,7 +1144,7 @@ def deploy(
explode_archive_atomic=None,
checksum=None,
by_checksum=False,
quote_parameters=None, # TODO: v0.10.0: change default to True
quote_parameters=True,
):
"""
Uploads a given file-like object
Expand All @@ -1164,17 +1163,9 @@ def deploy(
:param checksum: sha1Value or sha256Value
:param by_checksum: (bool) if True, deploy artifact by checksum, default False
:param quote_parameters: (bool) if True, apply URL quoting to matrix parameter names and values,
default False until v0.10.0
default True since v0.10.0
"""

if quote_parameters is None:
warn(
"The current default value of quote_parameters (False) will change to True in v0.10.0.\n"
"To ensure consistent behavior and remove this warning, explicitly set a value for quote_parameters.\n"
"For more details see https://github.com/devopshq/artifactory/issues/408."
)
quote_parameters = False

if fobj and by_checksum:
raise ArtifactoryException("Either fobj or by_checksum, but not both")

Expand Down Expand Up @@ -1848,11 +1839,7 @@ def write_bytes(self, data):
sha256 = hashlib.sha256(data).hexdigest()

fobj = io.BytesIO(data)
self.deploy(fobj, md5=md5, sha1=sha1, sha256=sha256, quote_parameters=False)
# TODO: v0.10.0 - possibly remove quote_parameters explicit setting
# Because this call never has parameters, it should not matter what it's set to.
# In this version, we set it explicitly to avoid the warning.
# In 0.10.0 or later, we can either keep it explicitly set to False, or remove it entirely.
self.deploy(fobj, md5=md5, sha1=sha1, sha256=sha256)
return len(data)

def write_text(self, data, encoding="utf-8", errors="strict"):
Expand Down Expand Up @@ -2189,12 +2176,7 @@ def copy(self, dst, suppress_layouts=False, fail_fast=False, dry_run=False):
md5=stat.md5,
sha1=stat.sha1,
sha256=stat.sha256,
quote_parameters=False,
)
# TODO: v0.10.0 - possibly remove quote_parameters explicit setting
# Because this call never has parameters, it should not matter what it's set to.
# In this version, we set it explicitly to avoid the warning.
# In 0.10.0 or later, we can either keep it explicitly set to False, or remove it entirely.

def move(self, dst, suppress_layouts=False, fail_fast=False, dry_run=False):
"""
Expand Down
42 changes: 6 additions & 36 deletions tests/unit/test_artifactory_path.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@ def test_deploy_file(self):
file.write("I am a test file")

# can't use pytest.mark.parametrize with unittest classes
for i, quote_params in enumerate((None, True, False)):
for i, quote_params in enumerate((True, False)):
responses.add(
responses.GET,
constructed_url,
Expand Down Expand Up @@ -1010,15 +1010,6 @@ def test_deploy_file(self):
props = path.properties
assert props == expected_properties

# TODO: v0.10.0 - None test will not be needed, warn test will not be needed
if quote_params is None:
self.assertWarnsRegex(
UserWarning,
r"^The current default value of quote_parameters \(False\) will change to True in v0\.10\.0\.\n"
r"To ensure consistent behavior and remove this warning, explicitly set a value for quote_parameters.\n"
r"For more details see https://github\.com/devopshq/artifactory/issues/408\.$",
)

# We are in a for loop, each iteration makes 3 mocked requests,
# and the one we want to do all these assertions on is the middle one.
request_index = (i * 3) + 1
Expand Down Expand Up @@ -1067,10 +1058,7 @@ def test_deploy_by_checksum_sha1(self):
json=self.file_stat_without_modification_date,
status=200,
)
self.path.deploy_by_checksum(sha1=self.sha1, quote_parameters=True)
# TODO: v0.10.0 - remove quote_parameters explicit setting
# These tests should allow the default value of the underlying method to be used.
# In this version, we set it explicitly to avoid the warning.
self.path.deploy_by_checksum(sha1=self.sha1)

self.assertEqual(len(rsps.calls), 1)
self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
Expand All @@ -1092,10 +1080,7 @@ def test_deploy_by_checksum_sha256(self):
json=self.file_stat_without_modification_date,
status=200,
)
self.path.deploy_by_checksum(sha256=self.sha256, quote_parameters=True)
# TODO: v0.10.0 - remove quote_parameters explicit setting
# These tests should allow the default value of the underlying method to be used.
# In this version, we set it explicitly to avoid the warning.
self.path.deploy_by_checksum(sha256=self.sha256)

self.assertEqual(len(rsps.calls), 1)
self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
Expand All @@ -1117,10 +1102,7 @@ def test_deploy_by_checksum_sha1_or_sha256(self):
json=self.file_stat_without_modification_date,
status=200,
)
self.path.deploy_by_checksum(checksum=self.sha1, quote_parameters=True)
# TODO: v0.10.0 - remove quote_parameters explicit setting
# These tests should allow the default value of the underlying method to be used.
# In this version, we set it explicitly to avoid the warning.
self.path.deploy_by_checksum(checksum=self.sha1)

self.assertEqual(len(rsps.calls), 1)
self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
Expand All @@ -1137,10 +1119,7 @@ def test_deploy_by_checksum_sha1_or_sha256(self):
json=self.file_stat_without_modification_date,
status=200,
)
self.path.deploy_by_checksum(checksum=self.sha256, quote_parameters=True)
# TODO: v0.10.0 - remove quote_parameters explicit setting
# These tests should allow the default value of the underlying method to be used.
# In this version, we set it explicitly to avoid the warning.
self.path.deploy_by_checksum(checksum=self.sha256)

self.assertEqual(len(rsps.calls), 1)
self.assertEqual(rsps.calls[0].request.url, self.artifact_url)
Expand All @@ -1163,12 +1142,7 @@ def test_deploy_by_checksum_error(self):
status=400,
)
with self.assertRaises(ArtifactoryException) as context:
self.path.deploy_by_checksum(
sha1=f"{self.sha1}invalid", quote_parameters=True
)
# TODO: v0.10.0 - remove quote_parameters explicit setting
# These tests should allow the default value of the underlying method to be used.
# In this version, we set it explicitly to avoid the warning.
self.path.deploy_by_checksum(sha1=f"{self.sha1}invalid")

self.assertEqual(str(context.exception), "Checksum values not provided")

Expand Down Expand Up @@ -1222,11 +1196,7 @@ def test_deploy_deb(self):
component="contrib",
architecture="amd64",
parameters={"z.additional": "param"},
quote_parameters=True,
)
# TODO: v0.10.0 - remove quote_parameters explicit setting
# These tests should allow the default value of the underlying method to be used.
# In this version, we set it explicitly to avoid the warning.

request_url = responses.calls[1].request.url
self.assertEqual(request_url, constructed_url)
Expand Down

0 comments on commit 2e92742

Please sign in to comment.