Skip to content

feat(ng-deploy): add option to specify firebaseProject (#2281) (#2063) #2366

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

Closed
wants to merge 5 commits into from
Closed

Conversation

george43g
Copy link
Contributor

Checklist

Description

As per #2063 , #2281, I have added the option for the target firebase project to be specified in angular.json. Previously, the builder would simply guess the target project based on the first target which contained the same name/host.

Code sample

In angular.json, you can now specify:

"deploy": {
          "builder": "@angular/fire:deploy",
          "options": {
            "firebaseProject": "projectNameOrAlias" // <-- This is the new option.
          }
}

When running ng deploy, the CLI will confirm which firebase project is being deployed to. This value is optional - if not supplied, builder will fall back to old behaviour of guessing. If the project value is invalid, it will fail gracefully with a descriptive error message.

This feature is incomplete because configurations are not yet working, however, this seems to be an issue with ng-cli. IE there is no way to have multiple configurations (production, dev, staging, etc..) yet.

@Splaktar
Copy link
Contributor

there is no way to have multiple configurations (production, dev, staging, etc..) yet

This isn't the case. See this angular.json config and these npm scripts.

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM as far as specifying a Firebase project to use, which is an extremely critical feature.

…targets

There has also been a minor code style change in how feedback is logged during deploy. My initial
commit used console.log, whereas the proper way uses context.logger... this has been fixed.
@george43g
Copy link
Contributor Author

there is no way to have multiple configurations (production, dev, staging, etc..) yet

This isn't the case. See this angular.json config and these npm scripts.

I should probably update some maintainers about this. So although it's possible using npm scripts, it wasn't possible using just a angular.json configuration. I've already submitted a request to fix the bug in the angular CLI and there's a pending merge: angular/angular-cli#17332

There are some ng deploy builders that actually rely on a --configuration flag to determine the build target, which is a mistake. Those setups will be broken by this update, I'll give them a heads up.

@Splaktar
Copy link
Contributor

Splaktar commented Mar 30, 2020

Ah, thank you for the clarification and bug report! Glad to see that Alan already has a PR to fix this in ng deploy.

@george43g
Copy link
Contributor Author

Hey, sorry I'm a bit new to this so maybe you can explain how it all works...
I made a couple of commits a few weeks back where I updated the docs and added a few small features.

So, it says that travisCI is still pending, but when I click on it it says it passed all tests... is this stopping the commits from being merged?

Have they been merged? It seems like it's still waiting for approval. Does it usually take a while?
And finally - it's also saying the branch is out of date - is there any point in merging the latest changes?
Sorry I'm not 100% sure what to do but I was hoping to start using the new features I wrote soon :)

@Splaktar
Copy link
Contributor

@george43g Travis says that the tests passed, but it seems like GitHub hasn't figured that out. If you do another push with some of the changes from @NothingEverHappens' suggestions, it should clear it up.

Your changes in this PR have not yet been merged. Getting changes merged can take weeks or months and requires a lot of patience.

The branch being out of date isn't an issue at this time. If it lists some conflicts, then you would need to merge in the latest changes and resolve the conflicts.

At this point, it's just waiting on @jamesdaniels or @davideast to do final approval and get it merged.

george43g added 2 commits May 1, 2020 14:22
As suggested in comments in #2366, I have made some small improvements
to existing changes pending in this PR.
@jamesdaniels
Copy link
Member

I'm working to cut 6.0.1 and support upcoming NG 10 release right now. Our CI/CD pipeline got messed up which is causing some backlog :( Will cut 6.1 with some new features shortly thereafter and I'm planning on getting this in.

@george43g
Copy link
Contributor Author

I'm working to cut 6.0.1 and support upcoming NG 10 release right now. Our CI/CD pipeline got messed up which is causing some backlog :( Will cut 6.1 with some new features shortly thereafter and I'm planning on getting this in.

Any chance we can get this in now? I'm working for a company as a devops role atm and we could really use this feature :D

@jamesdaniels
Copy link
Member

Solving conflicts and merging into #2647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants