Skip to content

Start using .travis.yml as a configure doctr.#137

Merged
asmeurer merged 7 commits into
drdoctr:masterfrom
Carreau:travis-config
Mar 14, 2017
Merged

Start using .travis.yml as a configure doctr.#137
asmeurer merged 7 commits into
drdoctr:masterfrom
Carreau:travis-config

Conversation

@Carreau

@Carreau Carreau commented Nov 9, 2016

Copy link
Copy Markdown
Collaborator

My guess is it will be easier with a subparser.

Just playing with it for now, see what's needed.

Comment thread doctr/__main__.py Outdated
def get_config():
"""
This load some configuration from the `travis.yml`, file is present,
`doctr` key if present.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two backquotes for RST.

Comment thread doctr/__main__.py Outdated

def get_config():
"""
This load some configuration from the `travis.yml`, file is present,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.travis.yml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

*if

@Carreau Carreau force-pushed the travis-config branch 8 times, most recently from 2cb985d to 0c097df Compare March 7, 2017 01:26
@Carreau Carreau changed the title [WIP] Start using .travis.yml as a configure doctr. Start using .travis.yml as a configure doctr. Mar 7, 2017
@Carreau

Carreau commented Mar 7, 2017

Copy link
Copy Markdown
Collaborator Author

Reviving this as part of the docathon.

I've cleaned it up a bit and will appreciate review.

@Carreau

Carreau commented Mar 7, 2017

Copy link
Copy Markdown
Collaborator Author

Oh, of course test are failing as the PR is from a branch not on this repo.

Comment thread .travis.yml Outdated
python -m doctr deploy --no-require-master --gh-pages-docs "docs-$TRAVIS_BRANCH" --built-docs docs/_build/html --key-path deploy_key.enc;
python -m doctr deploy --no-require-master --key-path deploy_key.enc --no-sync --command "echo test";
if [[ -z "${DOCTR_DEPLOY_ENCRYPTION_KEY}" ]]; then
echo "DOCTR_DEPLOY_ENCRYPTION_KEY not set, likely building a PR from untrusted repository";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We ought to be testing this in doctr itself. Is this happening to you here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hum, yes on my fork. Let me see if it was not rebased on the lastest.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"We ought" meaning we should. I don't know if we actually are (but if we aren't, we need to be). Someone actually reported this same error on SymPy, so I think there may be a real bug here.

In any case, let's fix this in a separate PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We do it when we decrypt the key on Travis: https://github.com/drdoctr/doctr/blob/master/doctr/travis.py#L51

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's only supposed to be called with canpush is True.

setup_deploy_key(keypath=keypath, key_ext=key_ext)
. So either we have a bug or Travis does (maybe $TRAVIS_PULL_REQUEST is not always set properly).

It would be nice if we could keep this as an error, because it is a legitimate error if someone didn't set up their .travis.yml correctly. Maybe there is a more robust way to detect pull requests than looking at the environment variables.

Let's open an issue for this.

Comment thread docs/changelog.rst
- ``setup_GitHub_push`` now takes a ``branch_whitelist`` parameter instead of
of a ``require_master``
- ``.travis.yml`` can be used to store some of doctr configuration in addition
to the command line flags. Write doctr configuration under the ``doctr`` key.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add :issue:`137` to these.

Comment thread docs/commandline.rst Outdated
The configuration parameters available from the ``.travis.yml`` file often
mirror their command line siblings except dashes ``-`` are replaced by
underscores ``_``. Some parameters are not available from configuration files
for security reasons.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we be more specific here? Are there any values that aren't the same after replacing - with _? Which flags aren't available for security reasons?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can we be more specific here? Are there any values that aren't the same after replacing - with _? Which flags aren't available for security reasons?

Hum, I don't remember. I'm using safe_load from Yaml so itm might be due to that. I'll rephrase.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All the parameters are paths (strings). Unexpected stuff can happen in YAML if you don't quote a string and it looks like another literal (like a float), but that's unlikely to happen here. I can't think of any other issues.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh, yeah. --token, if you allow a token option, it means pasting the token in clear in the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--token is a boolean flag. The token can only be accessed from a encrypted environment variable.

Comment thread docs/commandline.rst Outdated
Use a ``doctr`` section in your ``travis.yml`` file to store your doctr
configuration:

.. code::

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can set this as YAML language (though IIRC the highlighting might not actually work).

Comment thread docs/commandline.rst Outdated
- script: py.test
- doctr:
gh_pages_docs: 'documentation'
key_path : '/absolute/key/path.key'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Absolute key path? The key path should be relative to the github repo.

Comment thread docs/commandline.rst Outdated
- language: python
- script: py.test
- doctr:
gh_pages_docs: 'documentation'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will change with #157 (which will be merged soon).

Comment thread docs/commandline.rst Outdated
.. code::

- language: python
- script: py.test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You still have to actually call doctr deploy in the script though, right?

Comment thread doctr/__main__.py Outdated

config = travis_config.get('doctr',{})

if type(config) == list:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if not isinstance(config, dict)?

Comment thread doctr/__main__.py Outdated
This load some configuration from the ``.travis.yml``, if file is present,
``doctr`` key if present.
"""
with open('.travis.yml') as travis_config_file:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a check if the file exists.

Comment thread doctr/__main__.py Outdated

if args.command:
run(shlex.split(args.command))
run([os.path.expanduser(x) for x in shlex.split(args.command)])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this change here? I don't think we should do this.

Comment thread doctr/__main__.py Outdated

if type(config) == list:
raise ValueError('config is not a dict: {}'.format(config))
return travis_config.get('doctr',{})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

return config?

@Carreau Carreau force-pushed the travis-config branch 4 times, most recently from e2c1e31 to 5063e61 Compare March 8, 2017 23:58
@Carreau Carreau force-pushed the travis-config branch 5 times, most recently from 154e218 to 08b31cc Compare March 9, 2017 00:20
@Carreau

Carreau commented Mar 9, 2017

Copy link
Copy Markdown
Collaborator Author

Ok, I'm tired today, I did a lot of mistakes, but that should be all fixed.

@asmeurer

asmeurer commented Mar 9, 2017

Copy link
Copy Markdown
Member

I pushed the branch up to this repo so the tests can run from that.

@asmeurer

asmeurer commented Mar 9, 2017

Copy link
Copy Markdown
Member

We should probably add some configuration to our .travis.yml to test this (the existing tests will still work as they are as the command line flags override the .travis.yml keys).

@Carreau

Carreau commented Mar 9, 2017

Copy link
Copy Markdown
Collaborator Author

We should probably add some configuration to our .travis.yml to test this (the existing tests will still work as they are as the command line flags override the .travis.yml keys).

Done, I had to add opposites of some command line flags like --sync/--no-sync to be able to overwrite config file.

@asmeurer

asmeurer commented Mar 9, 2017

Copy link
Copy Markdown
Member

Cool. By the way, it's easier to review your changes if you do them as new commits.

@Carreau

Carreau commented Mar 14, 2017

Copy link
Copy Markdown
Collaborator Author

Cool. By the way, it's easier to review your changes if you do them as new commits.

Conflict addressed and push as multiple commits.

@asmeurer

Copy link
Copy Markdown
Member

Let's give it a try. Also self reminder we need to add pyyaml to the conda-forge recipe when we release.

@asmeurer asmeurer merged commit a63f788 into drdoctr:master Mar 14, 2017
@Carreau Carreau deleted the travis-config branch March 14, 2017 17:12
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.

3 participants