Thanks for looking into that, it looks like they are. The last time I looked into this, rust either wasn't installed or was a couple versions behind.
I'll remove that step.

Thanks for looking into that, it looks like they are. The last time I looked into this, rust either wasn't installed or was a couple versions behind. I'll remove that step.

newsch (Migrated from github.com) reviewed 2023-05-31 13:26:46 +00:00

.github/workflows/ci.yml

newsch (Migrated from github.com) commented

2023-05-31 13:26:46 +00:00

Yes, it is keyed by the rustc version, Cargo.lock/Cargo.toml hashes, and some other things as explained here.
The prefix-key still allows us to manually invalidate the cache when there is something wrong with the action.
I'll update the comment to make that clear.

Yes, it is keyed by the `rustc` version, `Cargo.lock`/`Cargo.toml` hashes, and some other things as [explained here](https://github.com/Swatinem/rust-cache#cache-details). The `prefix-key` still allows us to manually invalidate the cache when there is something wrong with the action. I'll update the comment to make that clear.

biodranik (Migrated from github.com) reviewed 2023-05-31 16:29:31 +00:00

biodranik (Migrated from github.com) left a comment

on: [push, pull_request]
Does this line launch the same actions twice when you create PR? How is it done in the main Organic Maps repo?

`on: [push, pull_request]` Does this line launch the same actions twice when you create PR? How is it done in the main Organic Maps repo?

.github/workflows/ci.yml

biodranik (Migrated from github.com) commented

2023-05-31 16:27:19 +00:00

          # Bump this to manually invalidate the build/dependency cache.

It's easier to read full sentences.

```suggestion # Bump this to manually invalidate the build/dependency cache. ``` It's easier to read full sentences.

biodranik (Migrated from github.com) commented

2023-05-31 16:28:15 +00:00

          # values explained here: https://github.com/Swatinem/rust-cache#cache-details

Is it automatically highlighted by some tool when <> are used?

```suggestion # values explained here: https://github.com/Swatinem/rust-cache#cache-details ``` Is it automatically highlighted by some tool when `<>` are used?

newsch (Migrated from github.com) reviewed 2023-05-31 19:37:18 +00:00

.github/workflows/ci.yml

newsch (Migrated from github.com) commented

2023-05-31 19:37:17 +00:00

I know some terminals and markup languages want urls bracketed, I think I got that habit from there. Not a requirement here.

newsch commented

2023-05-31 19:45:59 +00:00

(Migrated from github.com)

on: [push, pull_request] Does this line launch the same actions twice when you create PR? How is it done in the main Organic Maps repo?

It looks like all the workflows in organicmaps use only push or pull_request. My understanding is that the pull_request runs on the merge commit of the branch and the base, while push runs on the latest commit in the branch.
If you want only one, I think pull_request is what we want.

> `on: [push, pull_request]` Does this line launch the same actions twice when you create PR? How is it done in the main Organic Maps repo? It looks like all the workflows in organicmaps use only `push` or `pull_request`. My understanding is that the `pull_request` runs on the merge commit of the branch and the base, while `push` runs on the latest commit in the branch. If you want only one, I think `pull_request` is what we want.

newsch (Migrated from github.com) reviewed 2023-05-31 19:48:17 +00:00

.github/workflows/ci.yml

newsch (Migrated from github.com) commented

2023-05-31 19:48:17 +00:00

on: pull_request

```suggestion on: pull_request ```

newsch commented

2023-05-31 19:56:20 +00:00

(Migrated from github.com)

All the organicmaps workflows also have the workflow_dispatch event enabled, do you want that here too?

All the organicmaps workflows also have the [`workflow_dispatch` event](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch) enabled, do you want that here too?

biodranik (Migrated from github.com) reviewed 2023-05-31 20:39:13 +00:00

.github/workflows/ci.yml

biodranik (Migrated from github.com) commented

2023-05-31 20:35:54 +00:00

After reading the documentation I discovered something new to me: pull_request builds not the tip of the branch, but a (potential) merge commit with the master (branch where PR is targeted). So in reality we need both actions. You can see it in the log here: https://github.com/organicmaps/wikiparser/actions/runs/5126383678/jobs/9220795027#step:2:101

That's likely should be checked/fixed in the Organic Maps repo too.

biodranik (Migrated from github.com) commented

2023-05-31 20:39:10 +00:00

Can you please also add some exceptions to CI, like README, license, .gitignore?

newsch (Migrated from github.com) reviewed 2023-05-31 21:27:37 +00:00

.github/workflows/ci.yml

newsch (Migrated from github.com) commented

2023-05-31 21:27:37 +00:00

So in reality we need both actions.

If you want to avoid extra CI runs, I think using only pull_request is still reasonable.
It would be strange if something passes pull_request but fails push, and what I want to know is "Will this break things if it's merged", which is what pull_request does.

> So in reality we need both actions. If you want to avoid extra CI runs, I think using only `pull_request` is still reasonable. It would be strange if something passes `pull_request` but fails `push`, and what I want to know is "Will this break things if it's merged", which is what `pull_request` does.

biodranik (Migrated from github.com) approved these changes 2023-06-01 07:24:56 +00:00

biodranik (Migrated from github.com) left a comment

Good, thanks!

What's next step?

Good, thanks! What's next step?

newsch commented

2023-06-01 13:20:51 +00:00

(Migrated from github.com)

I'm working on a proof of concept to filter the articles by wikidata/wikipedia input, and output simplified html. Adding in multiple language support and connecting to the generator after that.