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

Fixing multi resource lifecycle hooks #1130

Merged
merged 13 commits into from Feb 8, 2019
Merged

Fixing multi resource lifecycle hooks #1130

merged 13 commits into from Feb 8, 2019

Conversation

Memeriaj
Copy link
Contributor

Description

Fixing lifecycle hooks for Multi-Site deploys as found by this issue. Essentially the lifecycle hooks only deal with the top level object and were never changed to deal with an array of configs. I tried to make this generic so that it'll be usable by other mutli resource deploys (which I don't think exist but RTDB could and we could technically do it for individual functions).

Scenarios Tested

Verified both single Site deploy and Multi-Site deploys. Also made sure that it worked if predeploy or postdeploy was an array or a single string.

Sample Commands

Everything used firebase deploy --only hosting but single Site deploy had a firebase.json of:

{
  "hosting": {
    "public": "public",
    "predeploy": ["echo 'single predeploy 1'", "echo 'single predeploy 2'"],
    "postdeploy": "echo 'single postdeploy 1' && echo 'single postdeploy 2'",

...

  }
}

Multi-Site deploy had a firebase.json of:

{
  "hosting": [{
    "target": "first",
    "predeploy": ["echo 'multi first predeploy 1'", "echo 'multi first predeploy 2'"],
    "postdeploy": ["echo 'multi first postdeploy 1'", "echo 'multi first postdeploy 2'"],

...

  },
  {
    "target": "second",
    "predeploy": ["echo 'multi second predeploy 1'", "echo 'multi second predeploy 2'"],
    "postdeploy": "echo 'multi second postdeploy 1' && echo 'multi second postdeploy 2'",
    "public": "public"
  }]
}

@Memeriaj Memeriaj self-assigned this Jan 31, 2019
@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Jan 31, 2019
@Memeriaj Memeriaj assigned mbleigh and bkendall and unassigned Memeriaj Jan 31, 2019
@coveralls
Copy link

coveralls commented Jan 31, 2019

Coverage Status

Coverage remained the same at 60.427% when pulling 0cd8308 on am-multi-predeploy into 05857c8 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.427% when pulling 4308c8a on am-multi-predeploy into adb77a5 on master.

Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can some unit tests be written for this functionality? Looks to me as if you could create some fake options with some echo statements that would succeed. Or maybe it could return a representation of the commands run that would be able to be verified? I'll leave it to you, but I think some tests here might be worth while.

Copy link
Contributor

@mbleigh mbleigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand @bkendall's encouragement to add tests, but I won't block on them here...I'd like to get this released and scaffolding deploy tests will take a non-trivial amount of time I imagine. If you somehow find time, lovely 😄

src/deploy/lifecycleHooks.js Outdated Show resolved Hide resolved
src/deploy/lifecycleHooks.js Outdated Show resolved Hide resolved
src/deploy/lifecycleHooks.js Outdated Show resolved Hide resolved
src/deploy/lifecycleHooks.js Outdated Show resolved Hide resolved
src/deploy/lifecycleHooks.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from me - comment from Michael to look at still...

@Memeriaj
Copy link
Contributor Author

Memeriaj commented Feb 7, 2019

Gave some tests a quick shot, both unit tests for just the lifecycle file and an integration test. I suspect creating meaningful tests are going to be quite a bit of work, and making them understandable and not brittle is going to require a fair amount of refactoring of how deploys are handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants