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

fix: github action workflow #76

Merged
merged 5 commits into from
Oct 8, 2021
Merged

fix: github action workflow #76

merged 5 commits into from
Oct 8, 2021

Conversation

JuanVqz
Copy link
Contributor

@JuanVqz JuanVqz commented Oct 6, 2021

No description provided.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 6, 2021

@ChaelCodes okay this will fix the problem with CI, I added some double-quotes and add the pry gem if you don't want them, I can remove it, should I?

BTW, it would be nice if you add the hacktoberfest label to this PR, hahaha LOL.

@ChaelCodes
Copy link
Owner

Double quotes and pry are fine. I think this is setup to only run CI on new pull requests and pushes to main.

@ChaelCodes
Copy link
Owner

Also, the Hacktoberfest label is on the repo, so all PRs approved or merged should apply! I'm happy to add a Hacktoberfest label and tag all your PRs with it though, for an extra level of security.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 6, 2021

Double quotes and pry are fine. I think this is setup to only run CI on new pull requests and pushes to main.

And I think it's what we want, didn't it?

Run the CI when somebody opens a PR and when it’s merged to the main branch it will run again.

@ChaelCodes
Copy link
Owner

Run the CI when somebody opens a PR and when it’s merged to the main branch it will run again.

I think it would be better if it ran whenever they pushed to their branch, so they could get to a green build.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 6, 2021

Run the CI when somebody opens a PR and when it’s merged to the main branch it will run again.

I think it would be better if it ran whenever they pushed to their branch, so they could get to a green build.

this will work as you expect, let me push again with a failing test just to confirm.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 6, 2021

As you can see, now the test build failed because my update. now I'm going to fix the failing test and we will see the test suite is green again.

  #before
   expect(profile.handle).not_to eq "ChaelChats"
  #After (with failing test)
   expect(profile.handle).to eq "ChaelChats"
Failures:

  1) /profiles PATCH /update with valid parameters with admin does not update the profile
     Failure/Error: expect(profile.handle).to eq "ChaelChats"

       expected: "ChaelChats"
            got: "ChaelCodes176"

       (compared using ==)
     # ./spec/requests/profiles_spec.rb:141:in `block (5 levels) in <main>'

Finished in 4.52 seconds (files took 2.58 seconds to load)
185 examples, 1 failure, 2 pending

BTW, you can see the history with the failing test over here.
image

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 6, 2021

the one which is failing it's because it takes the check.yml config from the main branch but once you merge this it will disappear.

Copy link
Owner

@ChaelCodes ChaelCodes left a comment

Choose a reason for hiding this comment

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

I'd really like for the line limiting checks to pushes to main to be removed.

Especially since we can't test the CI on this PR, and we're hoping that it does what we want to forked repo PRs.

push:
branches: [main]
Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy to add the pull_request key, but I don't want to limit CI to pushes to main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 🤖

Copy link
Owner

@ChaelCodes ChaelCodes left a comment

Choose a reason for hiding this comment

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

Let's try this out, and see if it works.

@ChaelCodes ChaelCodes merged commit 59a018d into ChaelCodes:main Oct 8, 2021
@ChaelCodes ChaelCodes added the hacktoberfest-accepted Accepted for Hacktoberfest label Oct 8, 2021
@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 8, 2021

Let's try this out, and see if it works.

it seems like it works well, 👍

@ChaelCodes ChaelCodes added this to the Hacktoberfest 2021 milestone Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants