Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ecs): Enable embedded artifacts for Deploy stage #8603

Merged
merged 2 commits into from
Oct 1, 2020

Conversation

atyutyunnik
Copy link
Contributor

This PR will allow for AWS ECS task definition artifacts embedded into the pipelines

@allisaurus allisaurus self-requested a review September 29, 2020 15:56
@allisaurus
Copy link
Contributor

allisaurus commented Sep 29, 2020

Thanks for submitting this, @atyutyunnik !
Change LGTM, though I'd like to see info on how this was tested (maybe screenshot of embedded artifact in stage + successful deployment). Also since this is an enhancement to the ECS provider, can you please update the commit msg prefix to "feat(ecs):" so it will show up correctly in future release notes/changelog?

@atyutyunnik
Copy link
Contributor Author

atyutyunnik commented Sep 29, 2020

Sometimes we use a heavily parameterized generic pipeline for multiple environments, instead of having specific pipelines:
2-DeployConfig

Then a task definition available as a JSON from cloudformation output:

Outputs:
...
  UiTaskDef:
    Value:
      Fn::Base64:
        !Sub
          |
          {
              "networkMode": "bridge",
              "family": "compute-test-ui",
              "placementConstraints": [],
              "executionRoleArn": "${InfrastructureStack.Outputs.EcsExecutionRole}",
...

can be transmitted as a parameter to such an initial deployment generic pipeline:
3-parameter

and embed it in Deploy ECS's json:
3-passing_TaskDef_as_base64_encoded_parameter

Of course, when editing such a Deploy ECS's task def artifact editor, it won't show anything (can't render the reference to pipeline's parameter; I think it should rather be grayed out and shown as is, if base64-text can't be decoded)

Yet deployment works just fine:

7-SuccessfulDeployment

@atyutyunnik atyutyunnik changed the title config(provider/aws): Enable embedded artifacts for Deploy stage feat(ecs): Enable embedded artifacts for Deploy stage Sep 29, 2020
@allisaurus
Copy link
Contributor

@atyutyunnik appreciate the info on testing! For the prefix change, unfortunately the commit message itself (not just the PR title) needs to be updated to reflect the new prefix. If you can do that and update from master, I think this is good to go.

@atyutyunnik
Copy link
Contributor Author

sorry for this misunderstanding and late response for I have a number of things going on at the moment. I hope I have done this time as prescribed

@allisaurus allisaurus added the ready to merge Reviewed and ready for merge label Oct 1, 2020
@mergify mergify bot merged commit 748da07 into spinnaker:master Oct 1, 2020
erikmunson pushed a commit that referenced this pull request Oct 1, 2020
748da07 feat(ecs): Enable embedded artifacts for Deploy stage (#8603)
61cf058 feat(ecs): add Amazon ECS Cypress tests
mergify bot pushed a commit that referenced this pull request Oct 1, 2020
…45 (#8612)

* chore(appengine): publish appengine@0.0.19

d36922b feat(provider/appengine): add deploy global configuration stage (#8599)

* chore(core): publish core@0.0.514

34bc7e9 feat(core/managed): switch to abbreviated timestamp format, add version timestamps (#8610)
012b004 feat(core/managed): show failed, pending bubbles for any constraint type (#8611)
7604b3d feat(webhooks): document stausJsonPath is now optional (#8608)
0463ecb feat(core/presentation): add useInterval hook (#8609)

* chore(ecs): publish ecs@0.0.265

748da07 feat(ecs): Enable embedded artifacts for Deploy stage (#8603)
61cf058 feat(ecs): add Amazon ECS Cypress tests

* chore(titus): publish titus@0.0.145

619ad9c (fix): Angular to react prop error (#8607)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Reviewed and ready for merge target-release/1.23
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants