add a components arg to checkout only select components #83

jedwards4b · 2018-03-29T17:05:20Z

Add optional argument to only process components listed on the command line
checkout_externals cam clm
will only process externals associated with the listed components cam and clm.
Also added a test of the new functionality.

User interface changes?: Yes
An additional command line option was added, previous functionality continues to be supported

Fixes: #80

Testing:
test removed:
unit tests: all pass
system tests:
manual testing: Tested with lists of components in Externals.cfg and with ones that were not there.

billsacks · 2018-04-03T20:52:49Z

I haven't been following the optional components design discussion, so I'm adding @gold2718 as a reviewer instead of myself.

@jedwards4b it appears that many of the system tests are failing. Let me know if you'd like a bit of an orientation to the manage_externals testing, if you haven't used it before.

jedwards4b · 2018-04-03T21:01:35Z

@billsacks yes, I would appreciate that. I ran them from the command line and all passed.

jedwards4b · 2018-04-03T22:07:48Z

@billsacks All of these tests fail on master as well.

jedwards4b · 2018-04-03T22:11:01Z

Sorry that last was incorrect, all tests pass on master and on my branch:

make utest
PYTHONPATH=..: python -m unittest discover --buffer --pattern 'test_unit_*.py'
........................................................................................................................................
----------------------------------------------------------------------
Ran 136 tests in 0.118s

OK
(env_python2) cheyenne3: /glade/p/work/jedwards/sandboxes/manage_externals/test
:) git branch
* jedwards/components_arg
  master
(env_python2) cheyenne3: /glade/p/work/jedwards/sandboxes/manage_externals/test

billsacks · 2018-04-03T22:49:14Z

@jedwards4b the issue is that there are other tests besides make utest: try make stest for the system tests, or just make test (the latter does utest & stest).

coveralls · 2018-04-04T00:12:46Z

Coverage decreased (-0.4%) to 91.463% when pulling a72642c on jedwards4b:jedwards/components_arg into d6423c6 on NCAR:master.

jedwards4b · 2018-04-04T00:54:13Z

@billsacks testing now passes - what's with the coverage check?

billsacks · 2018-04-04T01:04:21Z

Thanks, @jedwards4b . I'm happy with this once @gold2718 gives his okay.

@bandre-ucar added an automated code coverage check, which I think checks the coverage of all of the unit and system tests. I guess this is telling us that (much of) the new code isn't covered by any unit or system tests, hence the decrease in code coverage. I guess that, ideally, there would be a test added to cover this new code; I'm fine leaving it to you to determine whether it's worth doing so, based on (a) how critical this code is, and (b) how hard it would be to add tests for it.

jedwards4b · 2018-04-04T03:04:07Z

someone who knew the format of these tests could do this in no time, but I have to blunder my way through. This is really nicely written and documented code, it's too bad the help desk is so unresponsive.

billsacks · 2018-04-04T11:07:28Z

@jedwards4b great, thanks for adding those tests.

jedwards4b · 2018-04-04T12:33:32Z

I don't know what I need to do to improve the coverage further.

billsacks · 2018-04-04T13:02:26Z

It seems normal for coverage to vary up or down by small amounts like this, and I think @bandre-ucar has been accepting changes even for small decreases in coverage. So I don't think you need to do anything more. (Aside: I wonder if there's a way to configure coveralls to list it as passing as long as coverage doesn't decrease by more than ~ 1% or so - or maybe just make that informational rather than a failing check.)

gold2718

Mostly good but one show stopper for CAM moving forward.

gold2718 · 2018-04-10T15:14:46Z

manic/checkout.py


    load_all = False
-    if args.optional:
+    if args.optional or args.components:


It seems to me that if you use the new interface to specify a component (e.g., cam), this will force CAM to load all of its optional components which is certainly not the correct behavior. Is there a reason this has to be there?

Good point - I'll need to think about this a bit.

gold2718 · 2018-04-10T15:23:21Z

manic/checkout.py

-            source_tree.checkout(args.verbose, load_all)
+            if not args.components:
+                source_tree.checkout(args.verbose, load_all)
+            for comp in args.components:


I know this does the right thing but I feel adding an else before the loop would clarify that only one of these actions is intended to be taken.

Actually I originally had it that way and pylint flagged it as "too many branches."

pylint needs to get a life 👎 (but okay to leave it as is).

gold2718 · 2018-04-10T15:31:39Z

manic/externals_description.py


-    def __init__(self, model_data):
+    def __init__(self, model_data, components=None):
        """Convert the xml into a standardized dict that can be used to


If we are here, aren't we cfg format situation? If so, please fix this comment.

gold2718 · 2018-04-10T15:50:02Z

test/test_sys_checkout.py

+        overall, tree = self.execute_cmd_in_dir(under_test_dir,
+                                                self.status_args)
+        self._check_container_component_post_checkout2(overall, tree)
+


This is a good test but it seems that adding tests is not part of our standard PR template. Still, adding a mention of the new test is a good idea (IMHO).

gold2718 · 2018-04-10T18:14:55Z

@jedwards4b, will this new feature work if you have a dirty sandbox in an non-involved component? For instance, say components/pop is modified, can

./manage_externals/checkout_externals FMS

still work?

jedwards4b · 2018-04-10T20:21:04Z

@gold2718 I think that I've addressed all of your concerns. Thanks

coveralls · 2018-04-10T20:22:24Z

Coverage decreased (-0.3%) to 90.658% when pulling c1b5b09 on jedwards4b:jedwards/components_arg into 6923119 on ESMCI:master.

coveralls · 2018-04-10T20:22:24Z

Coverage decreased (-0.3%) to 90.658% when pulling c1b5b09 on jedwards4b:jedwards/components_arg into 6923119 on ESMCI:master.

coveralls · 2018-04-10T20:22:24Z

Coverage decreased (-0.3%) to 90.658% when pulling c1b5b09 on jedwards4b:jedwards/components_arg into 6923119 on ESMCI:master.

coveralls · 2018-04-10T20:22:24Z

Coverage decreased (-0.1%) to 90.795% when pulling 3f4c88f on jedwards4b:jedwards/components_arg into 6923119 on ESMCI:master.

jedwards4b · 2018-04-10T20:30:40Z

The answer to your dirty sandbox question is yes, sandboxes not directly involved in the request are ignored.

billsacks · 2018-04-10T21:24:01Z

@jedwards4b - I have merged this. Ben was making a tag for each merge to master, I think. Any feelings on whether we should continue that practice?

gold2718 · 2018-04-10T21:26:46Z

I am fine with fewer tags.

jedwards4b · 2018-04-10T21:31:05Z

I don't see a strong need for tags.

bandre-ucar requested a review from billsacks April 2, 2018 22:14

bandre-ucar assigned billsacks Apr 2, 2018

billsacks requested review from gold2718 and removed request for billsacks April 3, 2018 20:50

billsacks assigned billsacks and unassigned billsacks Apr 3, 2018

gold2718 requested changes Apr 10, 2018

View reviewed changes

jedwards4b added 7 commits April 10, 2018 14:13

add a components arg to checkout only select components

98a810d

only use components if populated

987df5a

fix pylint issue

91d4851

add a test

bf52ac6

another test

f78d60f

one more test

4fdf180

remove unneeded logic

c1b5b09

jedwards4b force-pushed the jedwards/components_arg branch from a72642c to c1b5b09 Compare April 10, 2018 20:16

fix comment

3f4c88f

gold2718 approved these changes Apr 10, 2018

View reviewed changes

billsacks merged commit b2f3ae8 into ESMCI:master Apr 10, 2018

jedwards4b deleted the jedwards/components_arg branch April 10, 2018 21:29

add a components arg to checkout only select components #83

add a components arg to checkout only select components #83

Uh oh!

Conversation

jedwards4b commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billsacks commented Apr 3, 2018

Uh oh!

jedwards4b commented Apr 3, 2018

Uh oh!

jedwards4b commented Apr 3, 2018

Uh oh!

jedwards4b commented Apr 3, 2018

Uh oh!

billsacks commented Apr 3, 2018

Uh oh!

coveralls commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedwards4b commented Apr 4, 2018

Uh oh!

billsacks commented Apr 4, 2018

Uh oh!

jedwards4b commented Apr 4, 2018

Uh oh!

billsacks commented Apr 4, 2018

Uh oh!

jedwards4b commented Apr 4, 2018

Uh oh!

billsacks commented Apr 4, 2018

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

gold2718 Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

jedwards4b Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

gold2718 Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

jedwards4b Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

gold2718 Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

gold2718 Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

gold2718 Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

gold2718 commented Apr 10, 2018

Uh oh!

jedwards4b commented Apr 10, 2018

Uh oh!

coveralls commented Apr 10, 2018

Uh oh!

coveralls commented Apr 10, 2018

Uh oh!

coveralls commented Apr 10, 2018

Uh oh!

coveralls commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedwards4b commented Apr 10, 2018

Uh oh!

billsacks commented Apr 10, 2018

Uh oh!

gold2718 commented Apr 10, 2018

Uh oh!

jedwards4b commented Apr 10, 2018

Uh oh!

Reviewers

Assignees

Labels

jedwards4b commented Mar 29, 2018 •

edited

Loading

coveralls commented Apr 4, 2018 •

edited

Loading

coveralls commented Apr 10, 2018 •

edited

Loading