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

Update readme and examples to use v2 #329

Merged
merged 2 commits into from
May 26, 2020
Merged

Update readme and examples to use v2 #329

merged 2 commits into from
May 26, 2020

Conversation

aiqiaoy
Copy link
Contributor

@aiqiaoy aiqiaoy commented May 26, 2020

@aiqiaoy aiqiaoy requested review from dhadka, joshmgross and madhurig May 26, 2020 16:47
@aiqiaoy aiqiaoy force-pushed the aiyan/v2-release-doc branch from 351f15e to 581312b Compare May 26, 2020 16:48
@aiqiaoy aiqiaoy requested a review from chrispat May 26, 2020 16:51
Copy link

@hashtagchris hashtagchris left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated
## Usage

### Pre-requisites
Create a workflow `.yml` file in your repositories `.github/workflows` directory. An [example workflow](#example-workflow) is available below. For more information, reference the GitHub Help Documentation for [Creating a workflow file](https://help.github.com/en/articles/configuring-a-workflow#creating-a-workflow-file).

### Inputs

* `path` - A directory to store and save the cache
* `path` - Directories to store and save the cache. Supports pattern matching, multipath and single file cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there plans to change this to paths since multiple paths are now supported or will this be kept as is?

Copy link
Member

Choose a reason for hiding this comment

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

This will be kept as-is to make it easier to upgrade to v2.

The three options are:

  1. Keep path as-is and make it easy to upgrade
  2. Rename to paths, break any users who try to change v1 to v2 and not know about this input name change.
  3. Support both path and paths, this might get confusing to users and we'd have to figure out what to do if a user specified both.

Copy link
Member

@joshmgross joshmgross left a comment

Choose a reason for hiding this comment

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

Left some minor comments, overall looks good.

The most important thing that's left is to update the path description in https://github.com/actions/cache/blob/master/action.yml#L5

examples.md Outdated
with:
path: ${{ github.workspace }}/.nuget/packages
key: ${{ runner.os }}-nuget-${{ hashFiles('**/packages.lock.json') }}
restore-keys: |
${{ runner.os }}-nuget-
```

With `actions/cache@v2` you can now exclude unwanted packages with [exclude pattern](https://github.com/actions/toolkit/tree/master/packages/glob#exclude-patterns)
Copy link
Member

Choose a reason for hiding this comment

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

"If you do not want to include them, consider to move the cache folder like below."

Should we remove the above recommendation since the wild card exclude pattern is a better approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it depends on how many packages the user wants to exclude. Maybe move the wild card example up to show that it's a better approach?

@smorimoto
Copy link
Contributor

I've opened an additional PR, which isn't very rush but may also be worth including in the initial release of v2. actions/toolkit#475

@aiqiaoy aiqiaoy force-pushed the aiyan/v2-release-doc branch from 6c5d976 to e6c708b Compare May 26, 2020 19:31
@aiqiaoy aiqiaoy merged commit b820478 into master May 26, 2020
@aiqiaoy aiqiaoy deleted the aiyan/v2-release-doc branch May 26, 2020 19:58
dhadka pushed a commit that referenced this pull request Jun 2, 2020
Update readme and examples to use v2
Copy link

@mkevre mkevre left a comment

Choose a reason for hiding this comment

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

aiyan/v2-release-doc

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.

6 participants