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

WIP - Update formula to follow template guidelines #54

Closed
wants to merge 0 commits into from

Conversation

javierbertoli
Copy link
Member

No description provided.

FORMULA Outdated Show resolved Hide resolved
FORMULA Outdated Show resolved Hide resolved

...

BREAKING CHANGE: With the removal of all of the `.sls` files under
Copy link
Member

Choose a reason for hiding this comment

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

Its unclear to me if template should be renamed to packages in these contrib docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Neither to me. Changing it, jic.

Copy link
Member

Choose a reason for hiding this comment

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

@noelmcloughlin @javierbertoli Across all semantic-release PRs, we've just left this as template, in order to minimise the number of changes as much as possible. It's not necessary here and it ends up leading to more false positives when diffing over time. Do you mind changing this back?

kitchen.yml Outdated Show resolved Hide resolved
Copy link
Member

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

LGTM. thanks @javierbertoli

Will you be splitting states into install.sls and clean.sls states for each package type (pkg, gem, pip, etc) in a future PR?

@javierbertoli javierbertoli changed the title Update formula to follow template guidelines WIP - Update formula to follow template guidelines Jun 17, 2019
Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

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

A couple of inline comments and a few more here.


...

BREAKING CHANGE: With the removal of all of the `.sls` files under
Copy link
Member

Choose a reason for hiding this comment

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

@noelmcloughlin @javierbertoli Across all semantic-release PRs, we've just left this as template, in order to minimise the number of changes as much as possible. It's not necessary here and it ends up leading to more false positives when diffing over time. Do you mind changing this back?

docs/README.rst Outdated

.. contents::
:local:
:local:

``packages``
------------
Copy link
Member

Choose a reason for hiding this comment

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

All of the states mentioned under Available states will need to use ^ as the heading symbol, otherwise they're not indented under the table of contents, as shown when viewing the current file:

@myii
Copy link
Member

myii commented Jul 30, 2019

@javierbertoli I've added four commits to this PR in my own fork, to test out ssf-formula and related improvements. You can see them here:

This includes the fixes I mentioned above inline and also gets the Debian test run working again by updating the nodejs version that needs to be installed:

Let me know if you want any/all of those commits added to this PR.

@myii
Copy link
Member

myii commented Jan 20, 2020

@javierbertoli #61 has been merged, so feel free to introduce your customisations from this PR on top of it.

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.

None yet

3 participants