Skip to content

Loads firebase secret from Taskcluster #868

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 7, 2019
Merged

Loads firebase secret from Taskcluster #868

merged 1 commit into from
Nov 7, 2019

Conversation

@jonalmeida
Copy link
Collaborator

@jonalmeida jonalmeida commented Aug 14, 2019 •
edited
Loading

Thought I'd take an attempt at loading the Firebase credentials @mitchhentges :) (needed for #865)

I think this might break sentry since the secret_index for sentry probably needs /sentry or that secret is moved to the r-b base path.

Pull Request checklist

Before merging checklist

  • Changelog: This PR includes a changelog entry or does not need one.

pocmo
pocmo reviewed Aug 14, 2019
View reviewed changes
automation/taskcluster/lib/tasks.py Outdated
command=command,
scopes=[
"secrets:get:{}".format(sentry_secret)
"secrets:get:{}".format(secret_index)
Copy link
Contributor

Choose a reason for hiding this comment

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

the scope here seems to be not correct. you will need a scope for every secret.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, if you follow what Fenix does, you should have a single secret for reference-browser/nightly :)

automation/taskcluster/lib/tasks.py Outdated
'python automation/taskcluster/helper/get-secret.py -s {} -k {} -f {}'.format(
sentry_secret, 'dsn', '.sentry_token'
),
secret_index, key, target_file
Copy link
Contributor

Choose a reason for hiding this comment

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

The secrets (with multiple key/value pairs) needs to exist here:
https://tools.taskcluster.net/secrets

project/mobile/reference-browser/sentry exists. project/mobile/reference-browser is not a secret. Either we fetch multiple secrets here or we create a secret that holds multiple values. Let's just follow what Fenix does. :)

mitchhentges
mitchhentges reviewed Aug 14, 2019
View reviewed changes
Copy link
Contributor

@mitchhentges mitchhentges left a comment

Choose a reason for hiding this comment

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

Thanks for working on this automation Jon, sorry I couldn't get around to it sooner

@jonalmeida
Copy link
Collaborator Author

@pocmo @mitchhentges taking a look at this again. I based it off the Fenix PR here. From what I understand, we do indeed have multiple secrets (firebase and sentry) but we don't have variants/build_type like Fenix does, so this should be correct, I think?

@mitchhentges
Copy link
Contributor

Since this draft PR was created, reference-browser and fenix have had their automation changed to use taskgraph (see the new automation under taskcluster/ here).

You'll want to change this PR to edit build-bundle/kind.yml and build/kind.yml. I think you can safely remove the os.makedirs(...) change, since that seems to be implemented in reference-browser now

@jonalmeida
Copy link
Collaborator Author

I was looking at the automation code in the old branch and didn't see taskgraph there, so I thought it was only in Fenix. Such a rookie mistake. 🤦‍♂

@jonalmeida jonalmeida force-pushed the firebase-key-support branch from 9e7a349 to 635f97f Compare November 6, 2019 23:28
@jonalmeida
Copy link
Collaborator Author

That was pretty easy. ✅

It seems a bit different from Fenix which does it in the transforms/build.py but I assume that's probably because of the built variants that it needs to be that way.

@jonalmeida jonalmeida marked this pull request as ready for review November 6, 2019 23:29
@jonalmeida jonalmeida requested review from a team as code owners November 6, 2019 23:29
mitchhentges
mitchhentges approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@mitchhentges mitchhentges left a comment

Choose a reason for hiding this comment

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

👌
Your UI tests might need the Firebase credentials as well, but you'll be more familiar with that than me I believe :)

@jonalmeida
Copy link
Collaborator Author

I think the QA team wrote those and iirc, that's a different Firebase key?

@jonalmeida jonalmeida merged commit df00818 into mozilla-mobile:master Nov 7, 2019
@jonalmeida jonalmeida deleted the firebase-key-support branch November 7, 2019 01:09
@mitchhentges
Copy link
Contributor

Makes sense 👍 thanks for PR-ing this!

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

Reviewers

2 more reviewers

@pocmo pocmo pocmo left review comments

@mitchhentges mitchhentges mitchhentges approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonalmeida @mitchhentges @pocmo