Set up Azure Pipelines by kaylangan · Pull Request #1 · kaylangan/react

kaylangan · 2019-02-20T19:09:59Z

kaylangan · 2019-02-20T19:51:58Z

@willsmythe @apawast Can you take a look at this? Also, I'm not really sure why the prettier step is failing...any ideas? Maybe it fails on PR builds but not CI?

We're also not that much faster than AppVeyor.

willsmythe · 2019-02-21T14:08:35Z

@kaylangan - I think the problem is that Azure Pipelines doesn't clone "master" and then checkout the commit being built like AppVeyor and CircleCI apparently do (which is why "master" is unknown). For reference, see the clone call in the AppVeyor log.

Without changing the existing logic, you should be able to do a git fetch origin master:master somewhere before this step, which should fetch the master branch and create a 'master' local ref.

Also, I would double check that their "get the list of changed files" logic works correctly for both CI and PR builds. For PR builds, HEAD will be a merge commit (merging of the source branch into the target branch) ... I'm not sure if this will cause problems for git merge-base or diff.

kaylangan · 2019-02-21T15:07:05Z

@willsmythe I had at first suspected that we don't fetch master, but this line made me think that we do. If not, what does that line actually mean then?

My suspicion was actually your second guess--that it's having trouble finding the merge-base of the merge commit and master and that this issue shows up in PR builds but not CI.

Either way, both are easy hypotheses to verify. I'll report back!

willsmythe · 2019-02-21T15:12:36Z

I’m definitely no Git expert … so you may be right. If you run “git branch -l”, you should get the list of local branches in the repo … which should validate (or invalidate) my suggestion 😊

kaylangan · 2019-02-21T20:48:37Z

@willsmythe What do you think of this structure for the jobs? Names of jobs/files still TBD, but I wanted to get feedback on the setup before getting in too deep.

willsmythe · 2019-02-21T21:05:09Z

@kaylangan - I like it. It's really clear when looking at the .azure-pipelines.yml what jobs will be run. I also like the way steps are reused. Other random things ...

Maybe use "common" instead of "setup" (in name of YAML file)
Maybe combine the Windows and Linux setup/common steps into a single file ... the git config steps won't hurt on Linux, it avoids an extra file, and reduces stuff (like step display names) getting out of sync across the different jobs.
It feels a little weird for the YAML files to live under "scripts" ... the alternative is a root-level .azure-pipelines folder that holds the main yaml and these secondary YAML files. Just a thought.

kaylangan · 2019-02-21T21:14:50Z

Maybe use "common" instead of "setup" (in name of YAML file)

Maybe combine the Windows and Linux setup/common steps into a single file ... the git config steps won't hurt on Linux, it avoids an extra file, and reduces stuff (like step display names) getting out of sync across the different jobs.

I like that. I'll update.

It feels a little weird for the YAML files to live under "scripts" ... the alternative is a root-level .azure-pipelines folder that holds the main yaml and these secondary YAML files. Just a thought.

Yeah, I went back and forth on this one. Are you thinking we put all the YAML files into the folder .azure-pipelines/ and then rename the scripts/circleci to scripts/ci or scripts/azure-pipelines?

willsmythe · 2019-02-21T21:22:05Z

It feels a little weird for the YAML files to live under "scripts" ... the alternative is a root-level .azure-pipelines folder that holds the main yaml and these secondary YAML files. Just a thought.

Yeah, I went back and forth on this one. Are you thinking we put all the YAML files into the folder .azure-pipelines/ and then rename the scripts/circleci to scripts/ci or scripts/azure-pipelines?

My gut reaction is that we have a root-level .azure-pipelines that contains all the YAML files and then we rename scripts/circleci to scripts/ci. Most of these scripts are generic, so it's not an illogical change. It just means changing a few scripts and the circleci YAML file. I think it's explainable/logical, but yah, it will generate more questions possibly ...

kaylangan · 2019-02-22T16:03:59Z

@willsmythe I'm going to put this aside for now. Some notes:

Perf: Similar to AppVeyor, and about twice as slow as CircleCI since we don't have caching. We could potentially split out the prod from the fire and fire prod tests, but we're still materially slower.
Failures: I don't think I can fix these since they need the coveralls token and GitHub tokens to work.
Naming: Ready for feedback here.
Tests: I'm reporting all but the build ones--perhaps those should be included?
Storing artifacts (TODO): https://github.com/kaylangan/react/blob/master/.circleci/config.yml#L44

kaylangan · 2019-03-11T18:46:38Z

@apawast Here's the react one. I kicked off a new build after refactoring some things--I'm expecting coveralls to fail--you can look at prior builds for details since it might take a bit before it hits the coveralls step.

willsmythe · 2019-03-12T13:48:13Z

+
+steps:
+  # master is not fetched by default and is needed to run prettier
+  - script: git fetch origin master:master


If you only need to fetch master for prettier, can you move this step next to / with the prettier step? For one, it would avoid a step showing up in every single job, especially if not all need it. It would also downplay this step, like if you moved it within your "script" step that invokes prettier.

see comment about adding prettier-ci to the package.json.

willsmythe · 2019-03-12T13:51:03Z

+  - script: set JEST_JUNIT_OUTPUT=.azure-pipelines/test-results/test-basic-windows.xml&&yarn test
+    displayName: 'Run tests'
+
+  - script: yarn prettier


Any idea why prettier is invoked differently in Windows vs. Linux? In Linux, the step has this:

node ./scripts/prettier/index

I don't think it's intentional; more a side effect of having developed the AppVeyor and Circle processes separately. yarn prettier runs node ./scripts/prettier/index write-changed which is probably only appropriate locally. So I would suggest either changing the line you highlighted to node ./scripts/prettier/index or adding another script to the package. I'm leaning adding prettier-ci to the package.json...what do you think?

If anything...if we add prettier-ci to the package, we can add the git fetch origin master:master to that script.

willsmythe · 2019-03-12T16:22:34Z

+      yarn test --maxWorkers=2
+    displayName: 'Run tests'
+
+  - script: ./scripts/circleci/check_license.sh


They may balk at this (and suggest this gets done in a separate PR), but what about renaming scripts/circleci to scripts/ci? One more thing in a theme of generalizing and becoming less tied to any one CI service.

Yeah, I was going to suggest it in the PR description but leave it out for now.

sizebot · 2019-05-29T17:21:25Z

	Warnings
⚠️	No bundle size information found. This indicates the build job failed.

Generated by 🚫 dangerJS

willsmythe · 2019-05-30T17:42:39Z

+#
+
+steps:
+  - script: |


Since this step is only needed on Windows, you might move it to the WindowsTests job or into a Windows-specific step template that gets referenced from the WindowsTests job.

(on Linux and macOS jobs, it's just unnecessary noise in the results/log)

willsmythe · 2019-05-30T17:45:44Z

+  - script: node ./scripts/tasks/flow-ci
+    displayName: 'Flow CI'
+
+  - script: |


I'd switch this to bash since it uses bash-specific keywords (which would fail if this step template is ever used on Windows). Of course, someone could always fix this later, but I've generally tried to use "bash" instead of just "script" when the script absolutely requires bash.

willsmythe · 2019-05-30T17:48:48Z

+  - script: yarn build
+    displayName: 'Build'
+
+  - script: set JEST_JUNIT_OUTPUT=.azure-pipelines/test-results/test-basic-windows.xml&&yarn test


I've been setting the env property of script when i need a variable to exist only for one step:

script: yarn test env: JEST_JUNIT_OUTPUT: '.azure-pipelines/test-results/test-basic-windows.xml'

kaylangan force-pushed the azure-pipelines branch from cd9ffc9 to 0a9c1cc Compare February 20, 2019 19:14

kaylangan force-pushed the azure-pipelines branch from 0a9c1cc to 352f4cf Compare February 20, 2019 20:37

willsmythe reviewed Feb 21, 2019

View reviewed changes

Comment thread .azure-pipelines.yml Outdated

kaylangan force-pushed the azure-pipelines branch from c282a13 to 747723f Compare February 21, 2019 20:46

willsmythe reviewed Feb 21, 2019

View reviewed changes

Comment thread .azure-pipelines.yml Outdated

Comment thread .azure-pipelines.yml Outdated

Comment thread scripts/azure-pipelines/windows-setup.yml

Comment thread scripts/azure-pipelines/linux-setup.yml Outdated

kaylangan force-pushed the azure-pipelines branch 2 times, most recently from 37efa33 to 99c8949 Compare March 11, 2019 18:46

kaylangan requested a review from apawast March 11, 2019 18:46

kaylangan force-pushed the azure-pipelines branch 2 times, most recently from 95427b5 to 7cb86e4 Compare March 11, 2019 21:30

willsmythe reviewed Mar 12, 2019

View reviewed changes

kaylangan force-pushed the azure-pipelines branch 2 times, most recently from 1f46f26 to 1ebf5ab Compare March 12, 2019 16:00

willsmythe reviewed Mar 12, 2019

View reviewed changes

kaylangan commented Mar 12, 2019

View reviewed changes

Comment thread .azure-pipelines/windows/basic-tests.yml Outdated

kaylangan force-pushed the azure-pipelines branch 4 times, most recently from 999c40b to a543745 Compare March 14, 2019 18:45

kaylangan changed the title ~~Set up Azure Pipelines for Windows~~ Set up Azure Pipelines Mar 28, 2019

kaylangan force-pushed the azure-pipelines branch from a543745 to 0e7f449 Compare May 29, 2019 16:46

Set up Azure Pipelines

69c0db3

kaylangan force-pushed the azure-pipelines branch from 0e7f449 to 69c0db3 Compare May 29, 2019 18:14

willsmythe reviewed May 30, 2019

View reviewed changes

Conversation

kaylangan commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaylangan commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willsmythe commented Feb 21, 2019

Uh oh!

kaylangan commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willsmythe commented Feb 21, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kaylangan commented Feb 21, 2019

Uh oh!

willsmythe commented Feb 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaylangan commented Feb 21, 2019

Uh oh!

willsmythe commented Feb 21, 2019

Uh oh!

kaylangan commented Feb 22, 2019

Uh oh!

kaylangan commented Mar 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sizebot commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaylangan commented Feb 20, 2019 •

edited

Loading

kaylangan commented Feb 20, 2019 •

edited

Loading

kaylangan commented Feb 21, 2019 •

edited

Loading

willsmythe commented Feb 21, 2019 via email •

edited

Loading