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

Automate deployment #546

Merged
merged 4 commits into from
Apr 10, 2017
Merged

Automate deployment #546

merged 4 commits into from
Apr 10, 2017

Conversation

jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Mar 16, 2016

Note: This patch depends on gh-545.

Introduce configuration to allow build servers provided by the Travis CI
service to execute the test generation tool and commit the resultant
files to the canonical upstream repository.

Enabling this workflow requires additional administrative work:

  1. Create an account with TravisCI
  2. Install the travis command-line utility
  3. Create a "deploy key" and an encrypted version using the command
    ./make.py github_deploy_key_enc
  4. Register the deploy key with the project's GitHub account
  5. Check the encrypted deploy key to the repository
  6. Configure the TravisCI service to automatically build this project

Ref #306

@leobalter
Copy link
Member

@jugglinmike now you can do this travis setup, right?

I'm +1 to enable it

@jugglinmike
Copy link
Contributor Author

@goyakin expressed concerns about granting push access to this repository to third parties (TravisCI in this case). This means we'll need a more formal "go ahead" before merging this.

I will note that the repository for the ECMA262 specification is already configured to grant access to that service.

@bterlson
Copy link
Member

I don't understand this concern. Travis, set up properly and without committing files you're not supposed to, is not a risk.

@jugglinmike
Copy link
Contributor Author

The concern related to granting push access to a third party. I have push access myself, but I don't feel I have the authority to share that with others.

Your comment makes it clear that this is not a problem, though :)

@bterlson
Copy link
Member

But travis only pushes on your behalf? But anyway, yes, agree this is not a problem :)

@leobalter
Copy link
Member

@bterlson @goyakin I don't have admin access to this repo so I can't proceed on the Travis configuration. Would you help me?

@leobalter
Copy link
Member

@tcare do you have admin access for this? It's time to get this landed :)

@tcare
Copy link
Member

tcare commented Mar 2, 2017

Alas I do not.. @bterlson or another admin will need to land this.

@leobalter
Copy link
Member

@jugglinmike Travis is activated. I was even running through the steps, but I need to find a definitive email address for the "maintainer". I'm not sure if we should use mine or @tcare's acc, I'll check if we can have some team managed address before.

Do you mind rebasing the PR for now? It might help in the sense travis should answer to the rebased changes.

make.py Outdated
OUT_DIR = os.environ.get('OUT_DIR') or 'test'
SRC_DIR = os.environ.get('SRC_DIR') or 'src'
UPSTREAM = os.environ.get('UPSTREAM') or '[email protected]:tc39/test262.git'
MAINTAINER = os.environ.get('MAINTAINER') or '[email protected]'
Copy link
Member

Choose a reason for hiding this comment

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

I just sent an email to @ecmageneva and @bterlson requesting an email acc for test262 @ ECMA

@leobalter leobalter mentioned this pull request Mar 13, 2017
@leobalter
Copy link
Member

I updated the original post to include a reference to #306

@leobalter
Copy link
Member

@jugglinmike we already have the email. \o/

@leobalter
Copy link
Member

I'm rebasing this for merge. Everyone, please hang on while I'll perform a forced push here.

jugglinmike and others added 2 commits April 7, 2017 12:08
Introduce configuration to allow build servers provided by the Travis CI
service to execute the test generation tool and commit the resultant
files to the canonical upstream repository.

Enabling this workflow requires additional administrative work:

1. Create an account with TravisCI
2. Install the `travis` command-line utility
3. Create a "deploy key" and an encrypted version using the command
   `./make.py github_deploy_key_enc`
4. Register the deploy key with the project's GitHub account
5. Check the encrypted deploy key to the repository
6. Configure the TravisCI service to automatically build this project
@leobalter leobalter force-pushed the automate-deployment branch from 81fa4de to b2bdb25 Compare April 7, 2017 16:08
@tcare
Copy link
Member

tcare commented Apr 7, 2017

LGTM

@leobalter
Copy link
Member

Before merging, I have some concerns I'm sorry I'm only bringing this now.

I'm +1 to automation, but I would prefer a different approach in here.

This PR is re-generating the tests from the master branch and pushing a commit with differences if any is detected. e.g.: someone pushed a PR changing the generated files directly this would revert it

This is good if we're in the master branch, but it can be silent and now send any clear signal on what happened and when it happened.

We could use travis for each PR to regenerate tests and not pass if it finds a git difference regenerating tests. We can use GitHub to block PRs if they are not passing in Travis.

For the master branch, we would only need a badge to tell everything remains ok. If it's failing, Travis will already send email notifications and signalize in the badge, rather than sending a new commit.

I'm up to apply these changes if it's ok.

@jugglinmike
Copy link
Contributor Author

I think I understand the changes you are requesting, but that they they
represent two distinct extensions to this patch:

  • Run the CI script to against all pull requests (and only create a commit when
    running on the master branch)
  • Extend the CI script to verify that generated files have not been modified
    directly

The first sounds like a logical extension to the current patch. It shouldn't be
too difficult to implement, but it should help catch template-related syntax
errors before they are merged to master.

The second change is a little more involved, though. The heuristic you
described (where the CI runs the test generation tool and looks for
modifications to exiting files) isn't quite complete. That's because we expect
a diff for modified files in cases where existing templates/cases are modified.
I think the validation you are describing is a good idea, but that implementing
it will require new logic.

I would like to implement the first change in this pull request and defer the
second change to a future pull request. The first change is in keeping with the
original scope of the pull request: automating previously-implemented
functionality. The second goes beyond that scope, which is particularly
undesirable for a change set that has been under consideration for almost a
year. At the same time, by configuring the build to run against all pull
requests, the first change will make implementation of that new validation step
easier.

What do you say?

@leobalter
Copy link
Member

leobalter commented Apr 9, 2017 via email

@jugglinmike
Copy link
Contributor Author

I think we're all set. Don't forget, though: we still need to generate an
encrypted deploy key. I don't have sufficient permissions to register such a
key with the project's GitHub repository, but I'd be happy to work with you
synchronously to get that set up.

@leobalter
Copy link
Member

Thanks!

I'll try it here and I'll ping you if necessary.

@leobalter
Copy link
Member

I believe I've got all the steps done.

I added the generated public key to https://github.com/tc39/test262/settings/keys as well and travis is already running.

@leobalter leobalter merged commit bcb7651 into tc39:master Apr 10, 2017
@leobalter
Copy link
Member

This is finally merged

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.

4 participants