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

Create rushjs workflow #201

Closed
wants to merge 4 commits into from
Closed

Create rushjs workflow #201

wants to merge 4 commits into from

Conversation

ChocPanda
Copy link

@ChocPanda ChocPanda commented Nov 17, 2019

Create a new starter workflow for rushjs

Thank you for sending in this pull request. Please make sure you take a look at the contributing file. Here's a few things for you to consider in this pull request:

  • Include a good description of the workflow.
  • Links to the language or tool will be nice (unless its really obvious)

In the workflow and properties files:

  • Includes a matching ci/properties/*.properties.json file.
  • Use title case for the names of workflows and steps, for example "Run tests".
  • The name of CI workflows should only be the name of the language or platform: for example "Go" (not "Go CI" or "Go Build")
  • Include comments in the workflow for any parts that are not obvious or could use clarification.
  • CI workflows should run push.
  • Packaging workflows should run on release with types: [created].

Some general notes:

  • Does not use an Action that isn't in the actions organization.
  • Does not send data to any 3rd party service except for the purposes of installing dependencies.
  • Does not use a paid service or product.

Create a new starter workflow for rushjs
@ChocPanda ChocPanda changed the title Create rushjs.yml Create rushjs workflow Nov 17, 2019
@ChocPanda
Copy link
Author

This is a starter workflow for people using the tool Rushjs. The workflow is based upon the travis ci configuration that is created using the rush init command and the existing nodejs workflow with an additional test step which would need to be added by the user.

@ChocPanda ChocPanda marked this pull request as ready for review November 17, 2019 23:15
ChocPanda and others added 2 commits November 17, 2019 23:24
Add a comment about rushjs test command
Copy link
Contributor

@andymckay andymckay left a comment

Choose a reason for hiding this comment

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

Thanks @ChocPanda and sorry for taking so long to getting around to reviewing this. I think there's a few improvements we can make to this and a question about the license for the logo.

@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything else that states this license. I went to https://github.com/microsoft/rushstack and found no license file or anything stating we can redistribute this logo. I think we'll be ok, but I need to check.


echo 'Testing...'
# This command does not ship with rushjs and would need to be implemented as a custom command https://rushjs.io/pages/configs/command_line_json/
node common/scripts/install-run-rush.js test --verbose
Copy link
Contributor

Choose a reason for hiding this comment

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

So this won't run out of the box, a user would have to do something?

run: |
set -e

echo 'Installing...'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend removing these echo and instead making steps for Install, Test and Build. They will show up individually in the UI on GitHub.

@ChocPanda
Copy link
Author

Thanks @andymckay , I will address the comments later today and try to get confirmation on the logo license

Co-authored-by: Andy McKay <andymckay@github.com>
@andymckay
Copy link
Contributor

Any luck with logo @ChocPanda

@github-actions
Copy link

This pull request has become stale and will be closed automatically within a period of time. Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants