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

Make locales extraction script work with and without token #14049

Closed

Conversation

diox
Copy link
Member

@diox diox commented Apr 23, 2020

Helps while we're waiting to have fully automated l10n extraction (mozilla/addons#5872)

@diox diox requested review from a team and eviljeff and removed request for a team April 23, 2020 15:11
@diox
Copy link
Member Author

diox commented Apr 23, 2020

Please do test locally. It should work from the container, but do note that it will attempt to checkout to master first.

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

I tried but it fails on jinja2.exceptions.TemplateSyntaxError: Encountered unknown tag 'load'. - which I thought was fixed in another commit?

# the following variables to the environment:
# - GITHUB_TOKEN (to the github token of addons-robot,
# talk to tofumatt or cgrebs)
# talk to @diox or @muffinresearch)
# - TRAVIS_REPO_SLUG="mozilla/addons-server"
Copy link
Member

Choose a reason for hiding this comment

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

Can't we provide this as a default in the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

As TRAVIS_BRANCH this is automatically present in travis

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking for local use so we don't have to define extra variables to make it run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'll make sure no variables are needed for manual usage

# the following variables to the environment:
# - GITHUB_TOKEN (to the github token of addons-robot,
# talk to tofumatt or cgrebs)
# talk to @diox or @muffinresearch)
# - TRAVIS_REPO_SLUG="mozilla/addons-server"
# - TRAVIS_BRANCH="master"
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be used at all (despite the erroring if it's not set).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is purely if used in travis to make sure it's not running against branches other than master

Copy link
Member

Choose a reason for hiding this comment

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

okay, that makes sense.

# the following variables to the environment:
# - GITHUB_TOKEN (to the github token of addons-robot,
# talk to tofumatt or cgrebs)
# talk to @diox or @muffinresearch)
# - TRAVIS_REPO_SLUG="mozilla/addons-server"
# - TRAVIS_BRANCH="master"

Copy link
Member

Choose a reason for hiding this comment

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

In init_environment it does git checkout master which depends on you having an up-to-date local master. Can we just git checkout upstream/master instead? (Even better if we can always checkout from the repo in TRAVIS_REPO_SLUG)

Copy link
Member Author

Choose a reason for hiding this comment

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

mmm in the case of travis, upstream doesn't exist as the remote... I'm going to try to steal some logic from https://github.com/mozilla/addons-frontend/blob/master/bin/run-l10n-extraction for this, pulling from the remote directly.


if [ "GITHUB_TOKEN" = "" ]
then
echo "No Github token provided, you'll have to push the branch and create the pull request yourself."
Copy link
Member

Choose a reason for hiding this comment

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

I understand why we can't create the pull request without the github API access but we could still push the branch like https://github.com/mozilla/addons-frontend/pull/9375/files#diff-cae1c8c05fe95719db14844a8f236ab1R76

@eviljeff
Copy link
Member

Anything that can be done to have mozilla/addons-frontend#9375 and this work in the same way would be a big improvement - right now the two scripts seem to make a lot of assumptions about git checkouts and branches and it's hard to remember the differences.

@diox
Copy link
Member Author

diox commented Apr 24, 2020

I tried but it fails on jinja2.exceptions.TemplateSyntaxError: Encountered unknown tag 'load'. - which I thought was fixed in another commit?

It was! which file was this ? Are you sure your master was up to date ?

@eviljeff
Copy link
Member

eviljeff commented Apr 24, 2020

I tried but it fails on jinja2.exceptions.TemplateSyntaxError: Encountered unknown tag 'load'. - which I thought was fixed in another commit?

It was! which file was this ? Are you sure your master was up to date ?

Forgot to do fetch upstream first. (can we do that too?).

It completed and then said:

Cleaning out obsolete messages.  See bug 623634 for details.
done.

*** Please tell me who you are.

Run

  git config --global user.email "[email protected]"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: unable to auto-detect email address (got 'olympia@0eca485b3639.(none)')

-this is being run in the container so does it have all the relevant git details?

@diox
Copy link
Member Author

diox commented Apr 24, 2020

Ah. Ok, so I should probably not even do the commit in that mode then I think, to be safe.

@diox
Copy link
Member Author

diox commented Apr 29, 2020

Going to rework this to use the same logic we use on the frontend.

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

Successfully merging this pull request may close these issues.

2 participants