Skip to content

Conversation

@iamjazzar
Copy link

@iamjazzar iamjazzar commented Jul 8, 2021

Change description

Run Sultan tests on this branch.

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Checklists

Development

  • Lint rules pass locally
  • Application changes have been tested thoroughly
  • Automated tests covering modified code pass

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
  • "Ready for review" label attached and reviewers assigned
  • Changes have been reviewed by at least one other contributor
  • Pull request linked to task tracker where applicable

@iamjazzar iamjazzar force-pushed the jazzar/sultan-dispatch branch 5 times, most recently from 9a9cbf6 to aa98718 Compare July 9, 2021 02:04
@iamjazzar iamjazzar requested a review from OmarIthawi July 9, 2021 02:05
thraxil
thraxil previously approved these changes Jul 9, 2021
OmarIthawi
OmarIthawi previously approved these changes Jul 9, 2021
@@ -0,0 +1,23 @@
on: push

Choose a reason for hiding this comment

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

The name of the file is pull_request.yml but it runs on push.

I recommend using the generic sultan-ci.yml file name.

Suggested change
on: push
on: push

Copy link
Author

Choose a reason for hiding this comment

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

I originally intended to run on pull requests, I totally forget about changing this.

@iamjazzar iamjazzar dismissed stale reviews from OmarIthawi and thraxil via c1bdfbf July 9, 2021 18:25
@iamjazzar iamjazzar force-pushed the jazzar/sultan-dispatch branch 4 times, most recently from 9a863c7 to da4e4bb Compare July 9, 2021 19:49
@iamjazzar iamjazzar force-pushed the jazzar/sultan-dispatch branch from da4e4bb to 5d09016 Compare July 9, 2021 19:49
@iamjazzar iamjazzar requested review from OmarIthawi and thraxil July 9, 2021 20:22
@@ -0,0 +1,22 @@
on: push
Copy link

@OmarIthawi OmarIthawi Jul 10, 2021

Choose a reason for hiding this comment

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

If you case you've forgot to run it on pull_request. The syntax below means that the ci will run on every pull request, in addition to push on the juniper branch.

This means that a PR merge, will trigger another build that allows us to actually save the image instead of just testing the pipeline.

Suggested change
on: push
on:
pull_request:
push:
branches:
- juniper

Copy link
Author

Choose a reason for hiding this comment

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

Triggering on pull_request won't allow us to fetch the branch name. Push actually works on merge too. Here's a proof:

Screen Shot 2021-07-10 at 9 54 21 AM

@iamjazzar iamjazzar merged commit 56ef6b8 into juniper Jul 10, 2021
@iamjazzar iamjazzar deleted the jazzar/sultan-dispatch branch July 10, 2021 16:28
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