Container2step #300
Container2step #300
Conversation
Improves error messages in container platform section
|
Did i break this? |
| """ | ||
| container_plat = CONTAINER_PLAT2[0] | ||
| yamlfile_path = f"{TEST_DIR}/{NM_EXAMPLE}/{YAMLFILE}" | ||
| create_makefile_script.makefile_create(yamlfile_path,container_plat,TARGET) |
There was a problem hiding this comment.
Try just passing create_makefile_script.makefile_create(yamlfile_path,CONTAINER_PLAT2,TARGET) instead. I'm pretty sure either the platformfre.py or yamlfre.py file is looking for a list that is passed. I used something like container_plat = CONTAINER_PLAT2[0] in some other tests mainly for assertions for directory names I think.
|
This needs an update to the schema submodule. NOAA-GFDL/gfdl_msd_schemas#17 |
|
was merged! is this still in need of further development? would suggest a quick and local edit: or w/e the exact |
|
This should be ready now. |
| except: | ||
| p["containerViews"] = False | ||
| raise NameError("Platform "+p["name"]+": You must specify the base container you wish to use to build your application") | ||
| ## Check if this is a 2 step (multi stage) build | ||
| try: | ||
| p["container2step"] |
There was a problem hiding this comment.
is this try/except clause just checking to see if a dictionary's attribute is defined at all? or is it about the boolean value assigned elsewhere?
if it's about being defined- how about using p.get("key"), then checking for None? or catching the KeyError explicitly?
There was a problem hiding this comment.
A bit of both I think? With some platforms, the container key doesn't even need to be defined. So the try/except checks for if it's even there under a certain platform's information (and not just that it's not defined, being None. If it doesn't exist, it'll set things as empty or False for the next if statement it goes through
There was a problem hiding this comment.
I think maybe this could be done a different way that's slightly easier to follow but my brain keeps going back and forth on logic and is confusing itself. Not sure what's the best way
| self.secondstage = ["FROM "+self.stage2base+" as final\n", | ||
| "COPY --from=builder "+self.src+" "+self.src+"\n", | ||
| "COPY --from=builder "+self.bld+" "+self.bld+"\n", | ||
| "ENV PATH=$PATH:"+self.bld+"\n"] |
There was a problem hiding this comment.
might i recommend f-strings for readability generally? with them, this reads like:
self.secondstage = [f"FROM {self.stage2base} as final\n",
f"COPY --from=builder {self.src} {self.src}\n",
f"COPY --from=builder {self.bld} {self.bld}\n",
f"ENV PATH=$PATH:{self.bld}\n"]
There was a problem hiding this comment.
I agree here, f-strings for the win
| def test_run_fremake_build_script_creation_container(): | ||
| ''' checks container build script creation from previous test ''' | ||
| assert Path("createContainer.sh").exists() | ||
|
|
||
| def test_run_fremake_dockerfile_creation_container(): | ||
| ''' checks dockerfile creation from previous test ''' | ||
| assert Path("Dockerfile").exists() | ||
|
|
||
| def test_run_fremake_checkout_script_creation_container(): | ||
| ''' checks checkout script creation from previous test ''' | ||
| assert Path(f"tmp/{CONTAINER_PLAT2[0]}/checkout.sh").exists() | ||
|
|
||
| def test_run_fremake_makefile_creation_container(): | ||
| ''' checks makefile creation from previous test ''' | ||
| assert Path(f"tmp/{CONTAINER_PLAT2[0]}/Makefile").exists() | ||
|
|
||
| def test_run_fremake_run_script_creation_container(): | ||
| ''' checks (internal) container run script creation from previous test ''' | ||
| assert Path(f"tmp/{CONTAINER_PLAT2[0]}/execrunscript.sh").exists() | ||
|
|
There was a problem hiding this comment.
these shouldn't be separate- lets do something like
def test_run_fremake_container():
run_fremake_script.fremake_run(
YAMLPATH, CONTAINER_PLAT2, TARGET, False, 1, True, False, VERBOSE)
assert all( [ condition1,
condition2,
...
conditionN ] )
| # tests container 2 stage build script/makefile/dockerfile creation | ||
| def test_run_fremake_container(): | ||
| '''run run-fremake with options for containerized build''' | ||
| run_fremake_script.fremake_run(YAMLPATH, CONTAINER_PLAT2, TARGET, False, 1, True, False, VERBOSE) |
There was a problem hiding this comment.
are there any return-codes to check?
There was a problem hiding this comment.
Gonna approve b.c. most of what i am pointing out is either style or readability driven, and the changes seem to function as intended given the test(s).
i'm not a fre make expert, but certain patterns in this subtool should be revisited and rewritten for maintainability's sake. E.g., class __init__ routines seem like a prime target.
| @@ -137,3 +150,17 @@ def getContainerImage(self,name): | |||
| for p in self.yaml: | |||
| if p["name"] == name: | |||
| return p["containerBase"] | |||
| def is2stepContainer(self,name): | |||
There was a problem hiding this comment.
Is this function used anywhere?
Describe your changes
Adds two-step container build capability to fre make
Issue ticket number and link (if applicable)
#266
Checklist before requesting a review