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

Add Azure Pipelines CI #2299

Merged
merged 12 commits into from
Mar 20, 2019
Merged

Add Azure Pipelines CI #2299

merged 12 commits into from
Mar 20, 2019

Conversation

damccorm
Copy link
Contributor

@damccorm damccorm commented Mar 8, 2019

The Change

This PR adds Azure Pipelines to perform CI builds and PR builds. Pipelines offers several key advantages:

  1. Single definition for all platforms (Linux/Windows/Mac) reduces the amount of time spent maintaining CI.
  2. Faster overall build times because of free parallelism - Pipelines offers 10 free concurrent jobs and unlimited build minutes for all OSS projects. Plus, for big projects like this one, Microsoft is happy to raise the concurrency limit, so we could have 15 or 20 parallel jobs. This should cut max build times to 4 or 4.5 minutes on Pipelines (it seems to vary a lot for Appveyor, but I've seen anything from 6 - 10 minutes in previous builds).
  3. Free MacOS builds - right now we're not building on MacOS at all, but some issues appear only on that OS.

Disclaimer

Full disclosure, I'm an engineer on the Pipelines team, but I also think Pipelines provides some pretty significant advantages here.

Additional Info

You can see that this works in my pipeline.

image

With test reporting!

image

@phated
Copy link
Member

phated commented Mar 8, 2019

This is actually something I've been considering.

I won't have any time today to dig in but I'll try to find time soon.

Can you remove the package-lock file that was generated? I haven't yet added the npmrc file that stops them from being generated.

@damccorm
Copy link
Contributor Author

damccorm commented Mar 8, 2019

Whoops, didn't realized I'd added that, taken care of 😄

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

@damccorm thanks for submitting this! I'm really excited about getting azure pipelines set up. I have a few comments/questions.


- task: PublishTestResults@2
inputs:
testResultsFiles: '**/xunit.xml'
Copy link
Member

Choose a reason for hiding this comment

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

So azure pipelines only speaks xunit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this input it does, but it can also use JUnit, NUnit, VSTest, xUnit, or cTest, we'd just need to change testResultsFiles to something like **/junit.xml. I believe if we want we could also do something like **/TEST-**.xml, this input really just works as a pattern matcher I think.

I used xunit since that matches the custom reporter I added (more on that below).

Copy link
Member

Choose a reason for hiding this comment

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

@damccorm It looks like mocha (even the version we use) ships with an XUnit reporter. Can we just use that? We can have an azure script that does mocha --async-only --reporter xunit

That way we can drop the extra reporter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works, the only downside is we don't get great output in the console.

What we get with multi-reporter:

image

What we get with xunit reporter:

image

I have this change in PR, if you're comfortable with that tradeoff I'll merge just let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually, on closer inspection, this doesn't get written to a file which is necessary for us to report results. We can pipe it out to a file, but that would mean we would get no output at all on the console.

Copy link
Member

Choose a reason for hiding this comment

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

We can use the -O output=test.xunit flag to specify the output file. I was thinking we could just run the test suites as separate phases, one with the spec reporter and one with the xunit reporter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me! Updated accordingly

.azure-pipelines-steps.yml Outdated Show resolved Hide resolved
.azure-pipelines-steps.yml Outdated Show resolved Hide resolved
.azure-pipelines-steps.yml Outdated Show resolved Hide resolved
.azure-pipelines.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@phated
Copy link
Member

phated commented Mar 12, 2019

@damccorm Just a few more things I followed up on. Also, do you have any guidance on how I signup and get this configured to work under a gulpjs organization for Azure Pipelines?

@phated
Copy link
Member

phated commented Mar 12, 2019

Oh, also - does Azure Pipelines support a .ci/ directory? I'd like to start moving things there if possible.

@damccorm
Copy link
Contributor Author

Oh, also - does Azure Pipelines support a .ci/ directory? I'd like to start moving things there if possible.

Sure, I moved it in there in my PR.

do you have any guidance on how I signup and get this configured to work under a gulpjs organization for Azure Pipelines?

The easiest way to get this done is probably to install the GitHub app after you've merged in this PR. That will walk you through the process of creating an organization and project. From there you can follow the getting started docs here.

One thing to call out is that since we moved the yaml to the .ci folder it won't be automatically discovered immediately when you create a build pipeline. That just means you'll want to select the Existing Azure Pipelines YAML file configuration and set /.ci/.azure-pipelines.yml as your path.

Let me know if anything is unclear as well, happy to help however I can!

@damccorm
Copy link
Contributor Author

One more thing - I noticed that both the organization names gulp and gulpjs are taken. I'm happy to transfer gulpci to you as an org name if you can't find a better one. It also looks like gulpjs is currently owned by team@gulpjs.com, so maybe your team already has one?

Regardless, installing the GitHub app is still the best way to get going.

@phated
Copy link
Member

phated commented Mar 13, 2019

@damccorm yeah, I did set one up a long time ago but I gave up when I ran into tons of XML. Is the YAML stuff new?

Can I use the existing team if I install it as a GitHub App? I just connected it and it looks like it works fine.

@damccorm
Copy link
Contributor Author

Is the YAML stuff new?

Yeah, it was added a little more than half a year ago as the default experience. It was part of a larger push (which is still ongoing) to offer a better OSS experience in particular.

Can I use the existing team if I install it as a GitHub App?

Yep! It should give you an opportunity to sign into an existing organization.

@phated
Copy link
Member

phated commented Mar 20, 2019

Sorry for the delay. This is looking great!

@phated phated merged commit 2e46ccd into gulpjs:master Mar 20, 2019
@phated
Copy link
Member

phated commented Mar 20, 2019

@damccorm I'm looking at the docs for secure variables and it seems to indicate there should be a 🔒 icon in the variables panel but I don't have that. How do I secure this coveralls token? Also, by using the coveralls token as a secure token, it says it won't be available to PRs - won't that break our coveralls Status API integration for PRs?

@damccorm
Copy link
Contributor Author

there should be a 🔒 icon in the variables panel but I don't have that

Are you looking at this page or somewhere else? This is what I see (with the secure checkbox column highlighted):

image

If not could you provide a little more info on what you're looking at? To get to that page, you can follow these steps:

Edit pipeline
image

Choose variables from the dropdown here:
image

Also, by using the coveralls token as a secure token, it says it won't be available to PRs - won't that break our coveralls Status API integration for PRs?

I missed that you were doing this. The answer is yes by default, but you can configure this behavior from the same menu (under the triggers tab)

image

If you do this, I would be careful though. The reason that secrets aren't enabled on forked builds is that someone could change a script that a secret is mapped into and do something malicious (e.g. send themselves the token). They can't change the yaml that gets executed though (or at least it won't get executed until merged), so a change you could make to ensure this doesn't happen is instead of calling npm run coveralls running the actual script (npm run cover && istanbul-coveralls). I'll add a PR to do this.

@phated
Copy link
Member

phated commented Mar 20, 2019

@damccorm thanks for following up. I have totally different variables view than you. Is it possible that I'm on a newer/older version of the UI because I signed up so long ago? I also noticed that other stuff in the documentation didn't match my UI.

@phated
Copy link
Member

phated commented Mar 20, 2019

@damccorm this is what I'm seeing for my "variables" screen
Screen Shot 2019-03-20 at 2 12 36 PM

@damccorm
Copy link
Contributor Author

Huh, that's weird, I haven't seen that view before (I joined Pipelines relatively recently though, so there's a chance it predates me). One thing that you could do is check to make sure the new UI didn't somehow get disabled in the preview features. For me you can access this here, hopefully this works for you as well:

image

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.

3 participants