-
Notifications
You must be signed in to change notification settings - Fork 171
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
Firebase Shell Plugin #331
Firebase Shell Plugin #331
Conversation
This PR is in Progress It is not ready for review I'm working on it |
Hello! Don't forget to also write an accompanying blog post on Hashnode with the tags 1Password and BuildWith1Password (not just putting #1Password in the text - but use the Hashnode tags in the CMS. :) The full instructions are here. |
I'm done with this plugin can you review it? |
Thanks for the heads up, @MukeshKumarBagaria. We'll get on this soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your submission, @MukeshKumarBagaria. There are a couple of things to address before we can move forward, including writing the config file importer and the corresponding test function. Please give it a try and let us know if you have any questions along the way! 😄
return schema.CredentialType{ | ||
Name: credname.AccessToken, | ||
DocsURL: sdk.URL("https://firebase.google.com/docs/cli#cli-ci-systems"), | ||
ManagementURL: sdk.URL("https://firebase.google.com/docs/cli#cli-ci-systems"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the ManagementURL because that URL is printed by the firebase login:ci
command. If it's a standard link and if you are familiar with it, let's include that.
Secret: true, | ||
Composition: &schema.ValueComposition{ | ||
Length: 53, | ||
Prefix: "dummy_firebase_", // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. This field is referring to the prefix like ghp_
in the case of GitHub Personal Access Tokens. Would you know if Firebase tokens have a similar prefix? If not, we can remove this field.
} | ||
|
||
var defaultEnvVarMapping = map[string]sdk.FieldName{ | ||
"FIREBASE_TOKEN": fieldname.Token, // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this TODO.
DefaultProvisioner: provision.EnvVars(defaultEnvVarMapping), | ||
Importer: importer.TryAll( | ||
importer.TryEnvVarPair(defaultEnvVarMapping), | ||
TryfirebaseConfigFile(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we actually importing from a configuration file? If yes, could we include an example file at /test-fixtures
path? Example from ngrok:
|
||
// TODO: Check if the platform stores the Access Token in a local config file, and if so, | ||
// implement the function below to add support for importing it. | ||
func TryfirebaseConfigFile() sdk.Importer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config file importer doesn't seem written. Let's make sure to do that, and let's also test it by actually running op plugin init firebase
. If the importer is properly written, there'd be a prompt to import those secrets.
func TestAccessTokenProvisioner(t *testing.T) { | ||
plugintest.TestProvisioner(t, AccessToken().DefaultProvisioner, map[string]plugintest.ProvisionCase{ | ||
"default": { | ||
ItemFields: map[sdk.FieldName]string{ // TODO: Check if this is correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO can be removed.
plugintest.TestProvisioner(t, AccessToken().DefaultProvisioner, map[string]plugintest.ProvisionCase{ | ||
"default": { | ||
ItemFields: map[sdk.FieldName]string{ // TODO: Check if this is correct | ||
fieldname.Token: "dummy_firebase_3bhfuelt31a99503j251bua8rov58m2example", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix to be fixed/removed once the correct prefix is known.
}, | ||
// TODO: If you implemented a config file importer, add a test file example in firebase/test-fixtures | ||
// and fill the necessary details in the test template below. | ||
"config file": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Config file importer test to be completed once the importer is written.
Name: "Firebase CLI", | ||
Runs: []string{"firebase"}, | ||
DocsURL: sdk.URL("https://firebase.google.com/docs/cli"), | ||
NeedsAuth: needsauth.IfAll( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's skip auth if the command contains the arg --token
because it takes precedence over envvars:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MukeshKumarBagaria, I noticed some changes since my last comment but they don't seem to address the previous suggestions. Is there something we can help with -- please let us know and we'd be happy to guide you on the next steps!
Hey @MukeshKumarBagaria - reaching out again, is there anything that we can help with, to move this forward? 🚀 |
Closing this because of inactivity - please do feel free to re-open if you'd like to pick this back up @MukeshKumarBagaria |
Overview
Added support for Firebase Shell plugin to authenticate using 1Password.
Type of change
How To Test
To test the changes:
firebase login:ci
to start the signin process.firebase <command> --token <token>
.Changelog
Authenticate Firebase CLI using 1Password for secure token storage.
Additional information