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

Build: implement build.jobs config file key #9016

Merged
merged 17 commits into from
Apr 4, 2022

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 15, 2022

Allow people to use build.jobs to execute pre/post steps.

build:
  jobs:
    pre_install:
      - echo `date`
      - python path/to/myscript.py
    pre_build:
      - sed -i **/*.rst -e "s|{version}|v3.5.1|g"

See a full example of a working config file: https://github.com/readthedocs/test-builds/blob/build-jobs/.readthedocs.yaml

Decisions made

  • build.jobs can't be used without build.os / build.tools. This is done on purpose to avoid maintaining old/deprecated features (config file v1 and v2 with build.image)
  • CWD path is the path where the repository was cloned
  • none of the "internal jobs" (checkout, install, build, etc) can be overwritten in this initial version. We will need a defined contract (e.g. metadata.yaml) to allow this
  • we are not escaping the commands the user wrote in the YAML. They are run as-is without modification
  • all user's commands are executed via the default Docker user (e.g. docs)
  • users can't execute commands as root
  • one, many or all the pre/post jobs can be defined
  • all user's commands receive environment variables

Closes #8201
Closes #5989

@humitos humitos changed the title Build: implment build.jobs config file key Build: implement build.jobs config file key Mar 15, 2022
Allow people to use `build.jobs` to execute pre/post steps.

```yaml
build:
  jobs:
    pre_install:
      - echo `date`
      - python path/to/myscript.py
    pre_build:
      - sed -i **/*.rst -e "s|{version}|v3.5.1|g"
```
@humitos humitos force-pushed the humitos/build-jobs-config-object branch from 74063f7 to 50980c7 Compare March 16, 2022 09:11
Put all the job steps under a helper called `run_build_job` to simplify the code.
Pass `READTHEDOCS_VERSION_TYPE` and `READTHEDOCS_VERSION_NAME` that were missed
because a merge conflict.

These variables were added in #8237
@humitos humitos force-pushed the humitos/build-jobs-config-object branch from 9adc275 to 1fcd354 Compare March 16, 2022 12:06
@humitos humitos marked this pull request as ready for review March 16, 2022 12:10
@humitos humitos requested a review from a team as a code owner March 16, 2022 12:10
@humitos humitos added the Status: blocked Issue is blocked on another issue label Mar 16, 2022
@humitos
Copy link
Member Author

humitos commented Mar 16, 2022

Blocking this PR because I'm going on vacation next week and it makes sense to help with deployment and QA.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a great start. I'm excited to get this out.

I'd like to see the docs included in this PR, which I think would help explain the use case. Also probably some kind of guide content or similar that shows what is possible here, with a few examples and suggestions.

readthedocs/config/config.py Show resolved Hide resolved
readthedocs/config/config.py Outdated Show resolved Hide resolved
readthedocs/config/models.py Show resolved Hide resolved
readthedocs/doc_builder/director.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/director.py Show resolved Hide resolved
readthedocs/doc_builder/director.py Show resolved Hide resolved
if job not in ("pre_checkout", "post_checkout"):
environment = self.build_environment

commands = getattr(self.data.config.build.jobs, job, [])
Copy link
Member

Choose a reason for hiding this comment

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

Can we use .get() instead of getattr()? Is it not a dict?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's not a dict. It's a BuildJobs instance. The config itself is an object and each of its attributes are objects 🤷🏼

Copy link
Member

Choose a reason for hiding this comment

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

all jobs should be already defined by this step, you can omit the getattr step.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd hrm, but how do you access the field name stored in the variable job?

Copy link
Member

Choose a reason for hiding this comment

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

ah, job is a variable, then the default from [] could be removed, that way we can also catch errors if we pass a wrong job name.

readthedocs/doc_builder/director.py Show resolved Hide resolved
readthedocs/doc_builder/director.py Outdated Show resolved Hide resolved
readthedocs/config/models.py Show resolved Hide resolved
readthedocs/doc_builder/director.py Outdated Show resolved Hide resolved
readthedocs/config/config.py Outdated Show resolved Hide resolved
Comment on lines +295 to +296
getattr(self.data.config.build, "jobs", None) is None
or getattr(self.data.config.build.jobs, job, None) is None
Copy link
Member

Choose a reason for hiding this comment

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

You can write this as if self.data.config.using_build_tools:, and all jobs should already be defined by this step, so no need for getattr on the right side of this expression.

readthedocs/doc_builder/director.py Show resolved Hide resolved
@humitos humitos removed the Status: blocked Issue is blocked on another issue label Mar 29, 2022
@humitos
Copy link
Member Author

humitos commented Mar 29, 2022

Hrm, I'd swear that I wrote a comment about this already. Here is my second attempt 😄

I made the requested changes and answered all the questions you had. I think this PR is ready to be merged after it gets approval. Let me know if there is anything else to be done before merging. I tested it locally with test-builds and it worked. However, I'm sure there may be more use cases to test. If you have one in mind, please create the branch in test-builds so we can test it and track it.

@ericholscher
Copy link
Member

@humitos I don't see any docs in this PR. Is the idea here that we'll ship the code without docs to be able to test it? I'd still like to understand a bit more the UX of the proposed code via docs, so perhaps we could make another PR that adds docs but isn't merged in the same release?

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks great to me, and I'm excited to get it out to our users. Next step is trying to tell people that they are supported & promote it's usage!

I'd love to have a KPI here on projects using this feature as a test for future "go to market" of new features. I expect this one will be quickly adopted if we do our jobs right. Can we do this easily in metabase these days?

readthedocs/config/models.py Show resolved Hide resolved
readthedocs/doc_builder/director.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/director.py Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Mar 30, 2022

I'd love to have a KPI here on projects using this feature as a test for future "go to market" of new features. I expect this one will be quickly adopted if we do our jobs right. Can we do this easily in metabase these days?

I've used "contains" over the Build._config text field in Metabase, but it's not super clean. However, we migrated that field to a JSON field now, so we should re-write those questions and use the correct query.

In any case, the short answer is "yes, we can get that data into metabase".

Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@humitos
Copy link
Member Author

humitos commented Mar 30, 2022

I don't see any docs in this PR. Is the idea here that we'll ship the code without docs to be able to test it? I'd still like to understand a bit more the UX of the proposed code via docs, so perhaps we could make another PR that adds docs but isn't merged in the same release?

Hrm, I thought of deploying this first but I wasn't able to shape it before yesterday. So, if we can write the docs and deploy everything together next week that sounds good to me. Otherwise, we can deploy just the code, for now, to start testing it. I wrote a basic reference documentation following what we have in that page and open a PR in #9056

I'm not sure what could doubt you may have regarding the UX but if you have any specific questions about it, let me know, so we can cover them in a guide. I think it will be good to cover common cases like "Unshallow repository" and "Incorrect version detection" at least.

@humitos humitos merged commit 050c886 into main Apr 4, 2022
@humitos humitos deleted the humitos/build-jobs-config-object branch April 4, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
4 participants