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

Design doc: improvements and new features for automation rules #7653

Closed
wants to merge 2 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 9, 2020

This is more like a collection of features and improvements,
since most of them are already being tracked in separated issues.

Let me know if there is anything else I should add/expand.

@stsewd stsewd requested a review from a team November 9, 2020 18:18
This is more like a collection of features and improvements,
since most of them are already being tracked in separated issues.
@stsewd stsewd force-pushed the automation-rules-improvements branch from ea08dcf to a3fa448 Compare November 9, 2020 18:48
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 is a good overview. What do you think is the best thing to work on first?

so an action shouldn't be able to be triggered only inside an automation rule.
This is so users aren't blocked if a rule didn't match a version,
or if they need to execute the action over existing versions.
- Automation rules are currently matched against new versions,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a better way to improve this might be removing the above restriction (eg. UI must be able to do it) and allow users to run automation rules across all their existing versions. This seems like a useful improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

A problem with that is if the user wants to change only one version or a couple, it needs to create a new rule with a pattern that only matches those versions, then run that rule against existing versions and delete it so new versions use a new rule.

We also need to think about how to place this element in the UI, but I guess that's the same amount of work (or maybe less) if we want to add a feature to enable it manually.

Copy link
Member

Choose a reason for hiding this comment

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

"Apply an automation rule on existing versions", is that what we are taking about here? If so, isn't it very similar to "Live feedback of versions that match a rule" feature? I mean, it seems it's exactly the same code. We could have a checkbox saying "Apply this rule to existing versions" when creating the automation rule.

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, but that has the problem that you'll need to write several rules and delete them just to match a couple of versions, instead of having the option of editing one by one, or in order to edit something you need to create a new rule. But I can see where that option could be useful too if you want to edit versions in bulk.


Use automation rules to decide which pull requests should be built,
this is for example only built branches that are going to be merged against an specific branch.
Or branch names that match a pattern, like (``^docs/.*``).
Copy link
Member

Choose a reason for hiding this comment

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

This seems really useful 👍

@stsewd
Copy link
Member Author

stsewd commented Nov 9, 2020

What do you think is the best thing to work on first?

I think the ones with no-design decisions or easy decisions 😄 Some would be:

  • Keep track of the n latest matches
  • Easy way to invert a match
  • Match which PRs should be built (here we don't need a separate UI element yet, as users can still trigger a new build by pushing to that branch if they got the pattern wrong)

Some other easy ones:

  • Live feedback of versions that match a rule (would we need to wait for the new templates?)
    We would need a new endpoint for this.
  • Set a version as stable (if we agree on having a project option like default version)

Harder ones that need thinking/design decision

  • Change the slug a version served from
  • Change privacy level of external versions
  • Set a version as stable

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I think automation rules are super powerful and I'm happy that we have this document mentioning the current state and improvements + new features.

IOM, I'd like to see these first:

  • slug rename
  • keep track of n matches
  • live feedback
  • decide what PRs to build


We do this with webhook exchanges (keeping the latest 10)
https://github.com/readthedocs/readthedocs.org/blob/f5b76e87a3b970d42228f5972ae9a50364c3c76c/readthedocs/integrations/models.py#L99-L111.
See https://github.com/readthedocs/readthedocs.org/issues/6393.
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful if we put all of these issues in a GH project to have an overview about how things are going.

so an action shouldn't be able to be triggered only inside an automation rule.
This is so users aren't blocked if a rule didn't match a version,
or if they need to execute the action over existing versions.
- Automation rules are currently matched against new versions,
Copy link
Member

Choose a reason for hiding this comment

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

"Apply an automation rule on existing versions", is that what we are taking about here? If so, isn't it very similar to "Live feedback of versions that match a rule" feature? I mean, it seems it's exactly the same code. We could have a checkbox saying "Apply this rule to existing versions" when creating the automation rule.

Improvements
------------

Keep track of the ``n`` latest matches
Copy link
Member

Choose a reason for hiding this comment

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

This is something that I've wanting for some time. I think it's super useful to keep track of what happened to understand why my versions have changed. Also, to debug mistakes on my own rules.


If automation rules are going to be used, users will need to be able to edit those versions.
This means having a place to list all external versions and allow to edit some properties,
privacy level is the only option that makes sense maybe also allow to de-activate it so docs aren't accessible.
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 not sure if it makes sense to "de-activate PR via automation rule". I think users can already disable this feature from the project admin. Is this different than the current disable option there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here the deactivating the version would delete the files from storage. Currently, we don't delete them. I think we may want to have this in .com, so the content isn't flying around

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow here. When a PR is merged, we do delete everything for that external version, including builds.

Can you elaborate what would be a good use case for the feature of "de-activating external versions via automated rules"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow here. When a PR is merged, we do delete everything for that external version, including builds.

We only delete the build logs, the documentation is kept in storage. But we went with another solution for now #7678


The models from automation rules already support passing enxtra arguments to be used by an action.
Before implementing this rule, we need to allow adding an alias for versions manually,
this would be by creating an alias (should the version be served from two slugs?)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to find a way to automatically create redirects on slug changes, so we don't break old links.

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 added a commit that referenced this pull request Nov 10, 2020
stsewd added a commit that referenced this pull request Nov 10, 2020
stsewd added a commit that referenced this pull request Nov 10, 2020
stsewd added a commit that referenced this pull request Nov 10, 2020
stsewd added a commit that referenced this pull request Nov 10, 2020
stsewd added a commit that referenced this pull request Nov 12, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
Allow to build an external version based on its source and base branch.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
- Use a namedtuple to pass more data about the external version around
- Automation rules can now receive extra kwargs to be passed down to the match and action functions
- Automation rules now return a tuple, the first element indicate if rule matched and the second is the return value from the action,
this was done, so we can know if the build was triggered or not for external versions.
- Moved the actions to the base class, since it doesn't look like we are going to have a different set of actions
- If the user doesn't have automation rules for external versions we just build everything as before,
  but if the user has at least one, we use the rules to trigger the build.

Ref #7653
stsewd added a commit that referenced this pull request Nov 16, 2020
- Use a namedtuple to pass more data about the external version around
- Automation rules can now receive extra kwargs to be passed down to the match and action functions
- Automation rules now return a tuple, the first element indicate if rule matched and the second is the return value from the action,
this was done, so we can know if the build was triggered or not for external versions.
- Moved the actions to the base class, since it doesn't look like we are going to have a different set of actions
- If the user doesn't have automation rules for external versions we just build everything as before,
  but if the user has at least one, we use the rules to trigger the build.

Ref #7653
@stsewd
Copy link
Member Author

stsewd commented Nov 23, 2020

Ok, after trying to implement automation rules for PRs #7664. I finally made use of the action_arg field :D, it was used to pass the pattern to match the base branch. But then I realized the limitation that we can only pass one extra argument, and maybe in the future we may want to pass more than one argument. So I was thinking we can change that field to be a json field, so it can have any number of values and in a structured way.

What other rules would require more than one extra argument?

  • Build a PR if the changed files match a pattern.

Here we want to also keep matching the base branch, in addition to a pattern for the changed files.

A rule would be something like, build all PRs where the base branch is "master" and the changed files match "^docs/*".
Also, maybe forget about matching the base branch and just match the pattern for the changed files?

But in any case, I think we want to make this a json field. We don't have any data there, so the migration should be plain and simple.

action_arg = models.CharField(
_('Action argument'),
help_text=_('Value used for the action to perfom an operation'),
max_length=255,
null=True,
blank=True,
)

stsewd added a commit that referenced this pull request Dec 7, 2020
@stale
Copy link

stale bot commented Jan 17, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jan 17, 2021
@stsewd stsewd removed the Status: stale Issue will be considered inactive soon label Jan 18, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jun 2, 2021
@stale stale bot closed this Jun 9, 2021
@stsewd stsewd removed the Status: stale Issue will be considered inactive soon label Jun 10, 2021
@ericholscher ericholscher deleted the automation-rules-improvements branch July 29, 2021 16:32
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