proposal for multiple configmaps #112 #113

skogie · 2020-08-28T12:50:16Z

WIP for issue #112
860 tests passed with tox -e test
Wasnt able to run the integration-test module it kept crashing so if there is more to be done here let me know.

tg90nor

Thanks for the contribution! Overall, this looks pretty good.

There should be some docs about this feature in https://github.com/fiaas/fiaas-deploy-daemon/blob/master/docs/v3_spec.md

I would also like to see some tests verifying the behaviour with 0, 1 and multiple configmaps defined for an app

Finally a small nit: I would like to see all occurences of config_maps renamed into configmaps, as that is how it is usually written when all lowercase

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py

skogie · 2020-08-28T15:16:50Z

The integration tests seem flaky on my machine if anyone could try to run them and give me feedback I would appreciate it!

tg90nor · 2020-08-28T16:36:47Z

Results (893.49s):
     140 passed
________________________________________________________________________________________________________________________ summary _________________________________________________________________________________________________________________________  integration_test: commands succeeded
  congratulations :)

gregjones

I like the idea, and it mostly looks good with just a couple of things I think should be clarified.

I also think Morten's comment from the issue, about also exposing them via volume-mounts makes sense - is there a reason not to do that?

docs/v3_spec.md

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py

Co-authored-by: Greg Jones <[email protected]>

skogie · 2020-09-01T10:06:47Z

I like the idea, and it mostly looks good with just a couple of things I think should be clarified.

I also think Morten's comment from the issue, about also exposing them via volume-mounts makes sense - is there a reason not to do that?

I was not aware of the volume-mounts, but yeah i should add those aswell!

…as-deploy-daemon into proposal-multiple-config-maps

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py

docs/v3_spec.md

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py

skogie · 2020-09-07T11:39:16Z

@mortenlj pushed some changes now, what do you think?

mortenlj · 2020-09-07T18:41:50Z

Starting to look good. The only thing I'd like to change is expanding the documentation with detailing of how precedence works for the added configmaps as we have discussed.

mortenlj · 2020-10-03T17:59:34Z

@skogie : Any chance you could sort out the documentation and maybe this can be merged?
@gregjones : What do you think, good?

gregjones

+1 on the docs, and an extra thing I think should be included. We should also make sure CI runs, right?

docs/v3_spec.md

skogie · 2020-10-05T17:51:55Z

I will try to get the remaining changes out tomorrow. Missing some tests and documentation.

mortenlj · 2020-11-11T11:59:36Z

What's the state of this PR, any movement?

skogie · 2020-11-16T14:13:13Z

@mortenlj I have been working on getting the tests to work locally but rn i have a round-trip of around 20 minutes per integration_test-run and its still not passing here, I think everything should be correct but could you try to run it on your end?

mortenlj · 2020-11-17T10:03:24Z

I have been working on getting the tests to work locally but rn i have a round-trip of around 20 minutes per integration_test-run and its still not passing here, I think everything should be correct but could you try to run it on your end?

@skogie : The last run seems to fail because of codestyle issues. Should be enough to fix those. To just run the codestyle tests, use tox -e codestyle (which is significantly quicker than waiting for the full set of tests).

skogie · 2020-11-17T11:13:12Z

@mortenlj I also have failing tests with tox -e integration_test, not sure if the CI server is running those?

skogie · 2020-11-17T11:59:45Z

Results (16.41s):00:33
     866 passed00:33
___________________________________ summary ____________________________________00:33
  codestyle: commands succeeded00:33
  test: commands succeeded00:33
  congratulations :)

tests seems to be running but the tox -e integration_test does not seem to be a part of the testing

mortenlj · 2020-11-17T12:05:43Z

tests seems to be running but the tox -e integration_test does not seem to be a part of the testing

Yes, we've hit #131

@oyvindio : Did you do any more research on that issue?

skogie · 2020-11-17T12:19:23Z

@mortenlj is it possible for you to run it locally to see if they actually pass, or if there is anything else i need to fix? I don't think they are working correctly on my machine

skogie · 2020-11-17T13:33:20Z

Fixed most of the tests now, but I still don't understand why these 2 are failing if someone could help me out: https://gist.github.com/skogie/4a786ff9ceec17a26466a3221819dbab and https://gist.github.com/skogie/0b480f01f46fd438e033134ef7a86de5

oyvindio · 2020-11-24T08:57:27Z

Yes, we've hit #131

@oyvindio : Did you do any more research on that issue?

It is on my list, but haven't been able to look into it further yet unfortunately. The only "workaround" I'm aware of to get the e2e tests to run before that issue is fixed, is to push the branch from the fork to a new branch on the source repository, which admittedly isn't a great solution

tests/fiaas_deploy_daemon/e2e_expected/v3full-deployment.yml

skogie · 2020-11-25T13:23:36Z

Would be nice to try to run it on the CI-server now if possible.

mortenlj · 2020-11-26T10:20:42Z

/sem-approve

mortenlj · 2020-11-26T10:42:56Z

I triggered a build directly in Semaphore after changing some settings which I hope doesn't expose our docker secrets to all PRs... 🙂
Also, I think perhaps, maybe, the above comment didn't work then, but should work now...

gregjones

This looks very nice now 👍
I noticed the copyright years in the new files aren't updated though, sorry to be picky. I've added the suggested changes with this review.

tests/fiaas_deploy_daemon/e2e_expected/v3-multiple-configmap-deployment.yml

tests/fiaas_deploy_daemon/e2e_expected/v3-no-extra-configmap-deployment.yml

tests/fiaas_deploy_daemon/e2e_expected/v3-single-extra-configmap-deployment.yml

tests/fiaas_deploy_daemon/specs/v3/data/examples/multiple_config_maps.yml

tests/fiaas_deploy_daemon/specs/v3/data/examples/single_extra_config_map.yml

Co-authored-by: Greg Jones <[email protected]>

mortenlj · 2021-01-15T08:58:12Z

@gregjones : This looks good now, doesn't it?

We still haven't figured out how to get Semaphore to build from forks automatically I guess, but I'll see if I can trigger something...

mortenlj · 2021-01-15T09:04:26Z

/sem-approve

gregjones

👍

skogie · 2021-03-01T09:41:21Z

@mortenlj looks like the build is failing now because some of the tasks are not running, can we try to trigger a new build and see if we can merge it?

mortenlj · 2021-03-01T10:27:39Z

/sem-approve

…oposal-multiple-config-maps

oyvindio · 2021-03-03T09:39:08Z

/sem-approve

mortenlj · 2023-01-12T09:17:50Z

I have a feeling this PR can be closed ... ?

proposal for multiple configmaps fiaas#112

32b0dcb

skogie requested a review from a team as a code owner August 28, 2020 12:50

fix newlines

c933e1c

skogie mentioned this pull request Aug 28, 2020

Proposal: Multiple config-map support for fiaas #112

Open

tg90nor reviewed Aug 28, 2020

View reviewed changes

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py Outdated Show resolved Hide resolved

Simen Skoglund added 2 commits August 28, 2020 17:15

Add docs and add integration tests

6b410fb

Remove unused file

c3ccfaa

gregjones suggested changes Aug 31, 2020

View reviewed changes

docs/v3_spec.md Outdated Show resolved Hide resolved

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py Outdated Show resolved Hide resolved

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py Outdated Show resolved Hide resolved

Update docs/v3_spec.md

fa81466

Co-authored-by: Greg Jones <[email protected]>

Simen Skoglund added 4 commits September 3, 2020 15:38

add configMap volumes for user-defined configMaps

348cf0b

put user-defined configMaps first and comment function

69b1b9a

Merge branch 'proposal-multiple-config-maps' of github.com:skogie/fia…

384d6fd

…as-deploy-daemon into proposal-multiple-config-maps

add new configMaps when making volume mounts

205c01f

skogie commented Sep 3, 2020

View reviewed changes

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py Show resolved Hide resolved

mortenlj reviewed Sep 5, 2020

View reviewed changes

more efficient appending of configmaps

8fc2c98

gregjones reviewed Oct 5, 2020

View reviewed changes

docs/v3_spec.md Outdated Show resolved Hide resolved

Update documentation and fix tests

55c390e

fix codestyle

29ef2f6

fix some integration tests

cf61a1c

mortenlj reviewed Nov 24, 2020

View reviewed changes

tests/fiaas_deploy_daemon/e2e_expected/v3full-deployment.yml Outdated Show resolved Hide resolved

remove -

7ec5ed4

skogie requested a review from gregjones November 26, 2020 12:26

gregjones reviewed Nov 26, 2020

View reviewed changes

skogie and others added 5 commits November 26, 2020 19:14

update copyright

dd94a02

Co-authored-by: Greg Jones <[email protected]>

update copyright

5aec742

Co-authored-by: Greg Jones <[email protected]>

update copyright

e591c6c

Co-authored-by: Greg Jones <[email protected]>

update copyright

171d068

Co-authored-by: Greg Jones <[email protected]>

update copyright

8d2ddf8

Co-authored-by: Greg Jones <[email protected]>

skogie requested a review from gregjones November 26, 2020 18:15

gregjones approved these changes Jan 18, 2021

View reviewed changes

Simen Skoglund added 2 commits March 1, 2021 16:30

Merge branch 'master' of github.com:fiaas/fiaas-deploy-daemon into pr…

cbe3fdc

…oposal-multiple-config-maps

Merge branch 'master' of github.com:fiaas/fiaas-deploy-daemon into pr…

f0b6b1e

…oposal-multiple-config-maps

tg90nor closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal for multiple configmaps #112 #113

proposal for multiple configmaps #112 #113

skogie commented Aug 28, 2020 •

edited

Loading

tg90nor left a comment

skogie commented Aug 28, 2020

tg90nor commented Aug 28, 2020

gregjones left a comment

skogie commented Sep 1, 2020

skogie commented Sep 7, 2020

mortenlj commented Sep 7, 2020

mortenlj commented Oct 3, 2020

gregjones left a comment

skogie commented Oct 5, 2020

mortenlj commented Nov 11, 2020

skogie commented Nov 16, 2020

mortenlj commented Nov 17, 2020

skogie commented Nov 17, 2020

skogie commented Nov 17, 2020

mortenlj commented Nov 17, 2020

skogie commented Nov 17, 2020

skogie commented Nov 17, 2020

oyvindio commented Nov 24, 2020

skogie commented Nov 25, 2020

mortenlj commented Nov 26, 2020

mortenlj commented Nov 26, 2020

gregjones left a comment

mortenlj commented Jan 15, 2021

mortenlj commented Jan 15, 2021

gregjones left a comment

skogie commented Mar 1, 2021

mortenlj commented Mar 1, 2021

oyvindio commented Mar 3, 2021

mortenlj commented Jan 12, 2023

proposal for multiple configmaps #112 #113

proposal for multiple configmaps #112 #113

Conversation

skogie commented Aug 28, 2020 • edited Loading

tg90nor left a comment

Choose a reason for hiding this comment

skogie commented Aug 28, 2020

tg90nor commented Aug 28, 2020

gregjones left a comment

Choose a reason for hiding this comment

skogie commented Sep 1, 2020

skogie commented Sep 7, 2020

mortenlj commented Sep 7, 2020

mortenlj commented Oct 3, 2020

gregjones left a comment

Choose a reason for hiding this comment

skogie commented Oct 5, 2020

mortenlj commented Nov 11, 2020

skogie commented Nov 16, 2020

mortenlj commented Nov 17, 2020

skogie commented Nov 17, 2020

skogie commented Nov 17, 2020

mortenlj commented Nov 17, 2020

skogie commented Nov 17, 2020

skogie commented Nov 17, 2020

oyvindio commented Nov 24, 2020

skogie commented Nov 25, 2020

mortenlj commented Nov 26, 2020

mortenlj commented Nov 26, 2020

gregjones left a comment

Choose a reason for hiding this comment

mortenlj commented Jan 15, 2021

mortenlj commented Jan 15, 2021

gregjones left a comment

Choose a reason for hiding this comment

skogie commented Mar 1, 2021

mortenlj commented Mar 1, 2021

oyvindio commented Mar 3, 2021

mortenlj commented Jan 12, 2023

skogie commented Aug 28, 2020 •

edited

Loading