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

Convert the CircleCI workflow to a GitHub Actions workflow #1221

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jk0
Copy link

@jk0 jk0 commented Feb 15, 2024

Summary

This pull request converts the CircleCI workflows to GitHub actions workflows. Github Actions Importer was used to convert the workflows initially, then I edited them manually to correct errors in translation.

Issues

  1. facebook/metro/build-and-deploy -> test-with-coverage
FAIL scripts/__tests__/babel-lib-defs-test.js
Full Error
Summary of all failing tests
FAIL scripts/__tests__/babel-lib-defs-test.js
  ● Babel Flow library definitions should be up to date
expect(received).toEqual(expected) // deep equality

- Expected  - 9
+ Received  + 0

@@ -898,13 +898,11 @@
      isImmutable(opts?: Opts): boolean;
      isImport(opts?: Opts): boolean;
      isImportAttribute(opts?: Opts): boolean;
      isImportDeclaration(opts?: Opts): boolean;
      isImportDefaultSpecifier(opts?: Opts): boolean;
-     isImportExpression(opts?: Opts): boolean;
      isImportNamespaceSpecifier(opts?: Opts): boolean;
-     isImportOrExportDeclaration(opts?: Opts): boolean;
      isImportSpecifier(opts?: Opts): boolean;
      isIndexedAccessType(opts?: Opts): boolean;
      isInferredPredicate(opts?: Opts): boolean;
      isInterfaceDeclaration(opts?: Opts): boolean;
      isInterfaceExtends(opts?: Opts): boolean;
@@ -1215,13 +1213,11 @@
      assertImmutable(opts?: Opts): void;
      assertImport(opts?: Opts): void;
      assertImportAttribute(opts?: Opts): void;
      assertImportDeclaration(opts?: Opts): void;
      assertImportDefaultSpecifier(opts?: Opts): void;
-     assertImportExpression(opts?: Opts): void;
      assertImportNamespaceSpecifier(opts?: Opts): void;
-     assertImportOrExportDeclaration(opts?: Opts): void;
      assertImportSpecifier(opts?: Opts): void;
      assertIndexedAccessType(opts?: Opts): void;
      assertInferredPredicate(opts?: Opts): void;
      assertInterfaceDeclaration(opts?: Opts): void;
      assertInterfaceExtends(opts?: Opts): void;
@@ -1573,17 +1569,12 @@
      Immutable?: VisitNode<BabelNodeImmutable, TState>,
      Import?: VisitNode<BabelNodeImport, TState>,
      ImportAttribute?: VisitNode<BabelNodeImportAttribute, TState>,
      ImportDeclaration?: VisitNode<BabelNodeImportDeclaration, TState>,
      ImportDefaultSpecifier?: VisitNode<BabelNodeImportDefaultSpecifier, TState>,
-     ImportExpression?: VisitNode<BabelNodeImportExpression, TState>,
      ImportNamespaceSpecifier?: VisitNode<
        BabelNodeImportNamespaceSpecifier,
-       TState,
-     >,
-     ImportOrExportDeclaration?: VisitNode<
-       BabelNodeImportOrExportDeclaration,
        TState,
      >,
      ImportSpecifier?: VisitNode<BabelNodeImportSpecifier, TState>,
      IndexedAccessType?: VisitNode<BabelNodeIndexedAccessType, TState>,
      InferredPredicate?: VisitNode<BabelNodeInferredPredicate, TState>,

  20 |   expect(contentByFilePath.size).toBe(2);
  21 |   for (const [filePath, content] of contentByFilePath) {
> 22 |     expect(await fsPromises.readFile(filePath, 'utf8')).toEqual(content);
     |                                                         ^
  23 |   }
  24 | });
  25 |

  at Object.toEqual (scripts/__tests__/babel-lib-defs-test.js:22:57)

How did you test this change?

I tested these changes in a forked repo.

https://fburl.com/workplace/f6mz6tmw

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 15, 2024
Copy link
Contributor

@robhogan robhogan left a comment

Choose a reason for hiding this comment

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

Thanks for this! Could you take a look at that publish.sh (maybe copy a port of it into .github) as well, and dry-run test the publishing workflow?

- run: echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}" >> ~/.npmrc
- name: Infer dist-tag and run npm run publish
if: steps.check_tag.outputs.valid == 'true'
run: "./.circleci/scripts/publish.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

This script relies on $CIRCLE_TAG, so I think it'll need porting unless GA populates that already.

# Reduce a semver tag name to a Metro's release branch naming convention, eg v0.1.2-alpha.3 -> 0.1.x
RELEASE_BRANCH=$(echo "$CIRCLE_TAG" | awk -F. '{print substr($1, 2) "." $2 ".x"}')

node-version: 18.12
cache: yarn
- name: Install Dependencies
run: npm install --force @babel/core
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be using Yarn to install dependencies so that yarn.lock is respected - that's the reason for the test failure you're seeing.

I'm not sure what the significance of @babel/core is here (and similarly --save-dev jest in jobs below)? yarn --frozen-lockfile --non-interactive --ignore-scripts is the corresponding line in our CircleCI config.yml, and we use that for all jobs:

command: yarn --frozen-lockfile --non-interactive --ignore-scripts

@bigfootjon
Copy link
Member

@robhogan can you just update this PR directly to do what you're asking? Then merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants