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

chore: add reusable plugin workflow #1

Merged
merged 8 commits into from
Dec 22, 2021
Merged

chore: add reusable plugin workflow #1

merged 8 commits into from
Dec 22, 2021

Conversation

Fdawgs
Copy link
Member

@Fdawgs Fdawgs commented Dec 13, 2021

Created a basic reusable GitHub Action workflow that can be used across all of the plugins in the Fastify org.

Also integrates changes to coveralls actions as mentioned in fastify/fastify#3328, and the latest fastify/github-action-merge-dependabot version.

Checklist

@Fdawgs Fdawgs self-assigned this Dec 13, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@climba03003
Copy link
Member

Should we remove node@10 and wait until fastify@4 for the reusable workflow integration?

README.md Outdated
- '*.md'

jobs:
call-reuseable-workflow:
Copy link
Member

Choose a reason for hiding this comment

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

We should add an example on how to use it with an external source, like mongodb

Copy link
Member

Choose a reason for hiding this comment

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

Do we know if this is possible to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% on what you mean @Eomm , sorry!

Do you mean using services like what is done in some of the Fastify repos such as fastify-postgres?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly!

I don't understand if this is working reading the docs

Copy link
Member Author

@Fdawgs Fdawgs Dec 20, 2021

Choose a reason for hiding this comment

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

From reading the docs, I have no idea either!

I imagine it would look like the workflow below, but can't confirm it would work.
I can open a test PR on fastify-postgres to see if it would work with my fork?

name: CI
on:
  push:
    paths-ignore:
      - 'docs/**'
      - '*.md'
  pull_request:
    paths-ignore:
      - 'docs/**'
      - '*.md'
jobs:
  call-reuseable-workflow:
    runs-on: ubuntu-latest
    services:
       postgres:
        image: postgres:11-alpine
        env:
            POSTGRES_USER: postgres
            POSTGRES_DB: postgres
            POSTGRES_PASSWORD: postgres
        ports:
            - '5432:5432'
        options: >-
            --health-cmd pg_isready --health-interval 10s --health-timeout 5s
            --health-retries 5
    steps:
        - uses: fastify/workflows/.github/workflows/plugins-ci.yml@v1
          with:
            generate-coverage: true

Edit: Unable to get it to work with my attempts.
From the errors returned:

  • Reusable workflows should be referenced at the top-level jobs.*.uses key, not within steps
  • You cannot define both uses and steps at the same time in a job
  • You cannot define both uses and services at the same time in a job

Copy link
Member Author

Choose a reason for hiding this comment

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

Think we'd have to create variations of the workflow to support use of services.

@Fdawgs Fdawgs requested a review from Eomm December 14, 2021 11:25
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Would you mind using two-space tag instead of 4 spaces on the yml files?

README.md Outdated
- '*.md'

jobs:
call-reuseable-workflow:
Copy link
Member

Choose a reason for hiding this comment

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

Do we know if this is possible to do?

@Fdawgs
Copy link
Member Author

Fdawgs commented Dec 18, 2021

Would you mind using two-space tag instead of 4 spaces on the yml files?

The 4 space style is due to me using Prettier for a lot of my stuff.
Standard, which seems to be the main style in the Fastify org, doesn't seem to support yml formatting.

I don't know if we need to decide on a standard format for yml and other file types?

@Eomm
Copy link
Member

Eomm commented Dec 18, 2021

Yes, standard is used to check js files.
I don't think that we need a yml files

I'm suggesting the 2 spaces because it fits better on mobile gh app when there is no pc at disposal, and to copy-cat the standard style as well

@Fdawgs
Copy link
Member Author

Fdawgs commented Dec 18, 2021

Yes, standard is used to check js files. I don't think that we need a yml files

I'm suggesting the 2 spaces because it fits better on mobile gh app when there is no pc at disposal, and to copy-cat the standard style as well

Cool beans, will take a pass at it on Monday when back in work.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

@Fdawgs
Copy link
Member Author

Fdawgs commented Dec 20, 2021

Yes, standard is used to check js files. I don't think that we need a yml files
I'm suggesting the 2 spaces because it fits better on mobile gh app when there is no pc at disposal, and to copy-cat the standard style as well

Cool beans, will take a pass at it on Monday when back in work.

Resolved by bc2a4f6

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

LGTM

Just one note: before cutting a v1 release I would test how the services keyword works as some most used plugins need to set up this feature

@Fdawgs Fdawgs merged commit 4658b2d into fastify:main Dec 22, 2021
@Fdawgs Fdawgs deleted the chore/initial-release branch December 22, 2021 12:19
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.

4 participants