Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
I grabbed the initial test portions of this merge proposal and put them into another at https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/327534 note, those do not have the tests for WarnIfNecessary -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:a2472853a66847a90b2e6892e3a22733518dbc8c https://jenkins.ubuntu.com/server/job/cloud-init-ci/431/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/431 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/431 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/431 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/431 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/431 Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/431/rebuild -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:092ea4027f241e6272588ae622b1ebd2692dde08 https://jenkins.ubuntu.com/server/job/cloud-init-ci/430/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/430 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/430 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/430 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/430 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/430 Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/430/rebuild -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:4999cd8ef760973e37a92b2755b7454849fbcb00 https://jenkins.ubuntu.com/server/job/cloud-init-ci/429/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/429/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/429/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/429/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/429/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/429/console Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/429/rebuild -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
I have to think some more on the 'FIXME' bits here. But i'd like feedback on teh added unit tests. -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:0c5f1e428c05f9eacb19e88fb9473ba8bf13992d https://jenkins.ubuntu.com/server/job/cloud-init-ci/424/ Executed test runs: FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/424/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/424/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/424/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/424/console FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/424/console Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/424/rebuild -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:4556902992eed670704cee83637e9870b6b946a2 https://jenkins.ubuntu.com/server/job/cloud-init-ci/393/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/393 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/393 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/393 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/393 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/393 Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/393/rebuild -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
I split the ds-identify portion out into https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324557 that way i can get it in now, and fix the ec2 warn portion itself with a nice test. -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
pre-approved on assumption of a new unit test for behavior, if the following
branch[1] lands before this one you could create a CiTestCase setting with_logs
= True and self.assertIn(" not warning", self.logs.getvalue())
References:
[1]
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324450
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Approve Diff comments: > diff --git a/tests/unittests/test_ds_identify.py > b/tests/unittests/test_ds_identify.py > index 9e14885..8559e1f 100644 > --- a/tests/unittests/test_ds_identify.py > +++ b/tests/unittests/test_ds_identify.py > @@ -210,6 +210,13 @@ class TestDsIdentify(CiTestCase): > mydata['files'][cfgpath] = 'datasource_list: ["NoCloud"]\n' > self._check_via_dict(mydata, rc=RC_FOUND, dslist=['NoCloud', > DS_NONE]) > > +def test_configured_list_with_none(self): > +"""If user set a datasource_list, that should be used.""" hah! > +mydata = copy.deepcopy(VALID_CFG['GCE']) > +cfgpath = 'etc/cloud/cloud.cfg.d/myds.cfg' > +mydata['files'][cfgpath] = 'datasource_list: ["Ec2", "None"]\n' > +self._check_via_dict(mydata, rc=RC_FOUND, dslist=['Ec2', DS_NONE]) > + > > def blkid_out(disks=None): > """Convert a list of disk dictionaries into blkid content.""" -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:194e15a6785f695ed5d228fdf6601837df3fb781 https://jenkins.ubuntu.com/server/job/cloud-init-ci/384/ Executed test runs: SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/384 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/384 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/384 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/384 SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/384 Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/384/rebuild -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
some responses in line. i'll fix some things too. thanks for review.
Diff comments:
> diff --git a/cloudinit/sources/DataSourceEc2.py
> b/cloudinit/sources/DataSourceEc2.py
> index 2f9c7ed..818a639 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
> @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval):
> return mode, sleep
>
>
> +def _is_explicit_dslist(dslist):
sure. I will fix.
bikeshed response: it is in the DataSourceEc2 module :)
> +if 'Ec2' not in dslist:
We do not support case insensitivity.
the string in datasource_list is passed to loading modules. it could
definitely be improved. but for now not supported.
if datasource_list is not a list, then this module will never be loaded.
> +return False
> +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist))
> +
> +
> def warn_if_necessary(cfgval, cfg):
> try:
> mode, sleep = parse_strict_mode(cfgval)
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> index 9e14885..8559e1f 100644
> --- a/tests/unittests/test_ds_identify.py
> +++ b/tests/unittests/test_ds_identify.py
> @@ -210,6 +210,13 @@ class TestDsIdentify(CiTestCase):
> mydata['files'][cfgpath] = 'datasource_list: ["NoCloud"]\n'
> self._check_via_dict(mydata, rc=RC_FOUND, dslist=['NoCloud',
> DS_NONE])
>
> +def test_configured_list_with_none(self):
> +"""If user set a datasource_list, that should be used."""
well, sort of.
It does that, but it also checks that if you put ['Ec2', 'None'],
then you get Ec2 used even if the data ('GCE') would pick a different cloud.
Ie, explicit user configuration of one entry or 2 including None is explicit
setting.
> +mydata = copy.deepcopy(VALID_CFG['GCE'])
> +cfgpath = 'etc/cloud/cloud.cfg.d/myds.cfg'
> +mydata['files'][cfgpath] = 'datasource_list: ["Ec2", "None"]\n'
> +self._check_via_dict(mydata, rc=RC_FOUND, dslist=['Ec2', DS_NONE])
> +
>
> def blkid_out(disks=None):
> """Convert a list of disk dictionaries into blkid content."""
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
On Mon, 22 May 2017, Ryan Harper wrote: > Generally looks fine, one question w.r.t confirming explict_dslist check > > Diff comments: > > > diff --git a/cloudinit/sources/DataSourceEc2.py > > b/cloudinit/sources/DataSourceEc2.py > > index 2f9c7ed..818a639 100644 > > --- a/cloudinit/sources/DataSourceEc2.py > > +++ b/cloudinit/sources/DataSourceEc2.py > > @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval): > > return mode, sleep > > > > > > +def _is_explicit_dslist(dslist): > > +if 'Ec2' not in dslist: > > +return False > > +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist)) > > if we know dslist is len == 2, then We could ensure it looks like: > ['Ec2', None] > > Right? That's exactly what we're looking for? Verus this [None, 'Ec2'] > which would pass this test but not be what we're looking for IIUC Yeah, I avoided checking for 'Ec2' because if this datasoruce is running, then Ec2 is in the list. But I think it makes more sense to be explicit Will cahnge as you sigguested. if dslist == ['Ec2', 'None'] or dslist == ['Ec2'] sm -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceEc2.py
> b/cloudinit/sources/DataSourceEc2.py
> index 2f9c7ed..818a639 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
There only seems to be one unit test which covers this module
(tests/unittests/test_datasource/test_aliyun.py). Is that right? Since we are
adding some level of functionality it'd be nice to cover this change in
functional behavior somewhere.
> @@ -64,7 +64,8 @@ class DataSourceEc2(sources.DataSource):
> LOG.debug("strict_mode: %s, cloud_platform=%s",
>strict_mode, self.cloud_platform)
> if strict_mode == "true" and self.cloud_platform ==
> Platforms.UNKNOWN:
> -return False
> +if not _is_explicit_dslist(self.sys_cfg.get('datasource_list')):
> +return False
>
> try:
> if not self.wait_for_metadata_service():
> @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval):
> return mode, sleep
>
>
> +def _is_explicit_dslist(dslist):
> +if 'Ec2' not in dslist:
> +return False
> +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist))
> +
> +
> def warn_if_necessary(cfgval, cfg):
> try:
> mode, sleep = parse_strict_mode(cfgval)
> @@ -276,6 +283,12 @@ def warn_if_necessary(cfgval, cfg):
> if mode == "false":
> return
>
> +dslist = cfg.get('datasource_list')
> +if _is_explicit_dslist(cfg.get('datasource_list')):
> +LOG.debug("mode=%s but explicit dslist (%s), not warning.",
> + mode, dslist)
> +return
> +
> warnings.show_warning('non_ec2_md', cfg, mode=True, sleep=sleep)
>
>
> diff --git a/tests/unittests/test_ds_identify.py
> b/tests/unittests/test_ds_identify.py
> index 9e14885..8559e1f 100644
> --- a/tests/unittests/test_ds_identify.py
> +++ b/tests/unittests/test_ds_identify.py
> @@ -210,6 +210,13 @@ class TestDsIdentify(CiTestCase):
> mydata['files'][cfgpath] = 'datasource_list: ["NoCloud"]\n'
> self._check_via_dict(mydata, rc=RC_FOUND, dslist=['NoCloud',
> DS_NONE])
>
> +def test_configured_list_with_none(self):
> +"""If user set a datasource_list, that should be used."""
This docstring should really be
"When a datasource_list already contains None, None is not added to the list.
The test just above it is already asserting datasource_list w/out None results
in a list w/ None at the end.
> +mydata = copy.deepcopy(VALID_CFG['GCE'])
> +cfgpath = 'etc/cloud/cloud.cfg.d/myds.cfg'
> +mydata['files'][cfgpath] = 'datasource_list: ["Ec2", "None"]\n'
> +self._check_via_dict(mydata, rc=RC_FOUND, dslist=['Ec2', DS_NONE])
> +
>
> def blkid_out(disks=None):
> """Convert a list of disk dictionaries into blkid content."""
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Diff comments:
> diff --git a/cloudinit/sources/DataSourceEc2.py
> b/cloudinit/sources/DataSourceEc2.py
> index 2f9c7ed..818a639 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
> @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval):
> return mode, sleep
>
>
> +def _is_explicit_dslist(dslist):
bikeshed disclaimer: _is_explicit_dslist is really only checking if Ec2 is
explicitly configured, why not rename _is_ec2_explicit_datasource? Then there
is no question about what this function does.
> +if 'Ec2' not in dslist:
Also, is it possible that dslist comes in configured as None via either an
improper "datasource_list: " or a config.cfg that doesn't define a
datasource_list at all so our (self.sys_cfg.get('datasource_list') gives us
None?
If so, then our 'Ec2' in dslist check would stacktrace
> +return False
> +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist))
> +
> +
> def warn_if_necessary(cfgval, cfg):
> try:
> mode, sleep = parse_strict_mode(cfgval)
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Diff comments: > diff --git a/cloudinit/sources/DataSourceEc2.py > b/cloudinit/sources/DataSourceEc2.py > index 2f9c7ed..818a639 100644 > --- a/cloudinit/sources/DataSourceEc2.py > +++ b/cloudinit/sources/DataSourceEc2.py > @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval): > return mode, sleep > > > +def _is_explicit_dslist(dslist): > +if 'Ec2' not in dslist: Should we support case-insensitivity for the configured datasources? ec2 EC2 Ec2? > +return False > +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist)) > + > + > def warn_if_necessary(cfgval, cfg): > try: > mode, sleep = parse_strict_mode(cfgval) -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master
Generally looks fine, one question w.r.t confirming explict_dslist check Diff comments: > diff --git a/cloudinit/sources/DataSourceEc2.py > b/cloudinit/sources/DataSourceEc2.py > index 2f9c7ed..818a639 100644 > --- a/cloudinit/sources/DataSourceEc2.py > +++ b/cloudinit/sources/DataSourceEc2.py > @@ -266,6 +267,12 @@ def parse_strict_mode(cfgval): > return mode, sleep > > > +def _is_explicit_dslist(dslist): > +if 'Ec2' not in dslist: > +return False > +return (len(dslist) == 1 or (len(dslist) == 2 and 'None' in dslist)) if we know dslist is len == 2, then We could ensure it looks like: ['Ec2', None] Right? That's exactly what we're looking for? Verus this [None, 'Ec2'] which would pass this test but not be what we're looking for IIUC > + > + > def warn_if_necessary(cfgval, cfg): > try: > mode, sleep = parse_strict_mode(cfgval) -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324274 Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1683038-ec2-no-warn-on-explicit into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
