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

GitHub action #103

Merged
merged 2 commits into from
Jan 26, 2021
Merged

GitHub action #103

merged 2 commits into from
Jan 26, 2021

Conversation

rndmh3ro
Copy link
Member

No description provided.

Sebastian Gumprich added 2 commits January 26, 2021 12:24
Signed-off-by: Sebastian Gumprich <sebastian.gumprich@t-systems.com>
Signed-off-by: Sebastian Gumprich <sebastian.gumprich@t-systems.com>
@schurzi schurzi merged commit abf50a2 into master Jan 26, 2021
@schurzi schurzi deleted the github_action branch January 26, 2021 11:27
@deric4
Copy link
Member

deric4 commented Jan 26, 2021

@rndmh3ro @schurzi is there any more context on this PR somewhere? This is sort of a breaking change isn't it since none of the test actions are passing? The rubocop changes were discussed briefly in #90 .

@rndmh3ro
Copy link
Member Author

We changed the travis tests with github actions. There was only some internal discussion. We plan to fix the tests, too.

Why do you think this is a breaking change? The code itself did not change.

@schurzi
Copy link
Contributor

schurzi commented Jan 26, 2021

my rationale for merging this was, that it may be not a great idea to change the complete CI workflow and do a lot of syntactic code changes in one PR. We are already working on fixng the lint recommendations. :)

@deric4
Copy link
Member

deric4 commented Jan 26, 2021

Why do you think this is a breaking change? The code itself did not change.

Prior to this change the convention seemed to be that all the Checks must pass (DCO and Travis). Now that travis has been removed, there are 3 failing Checks now for the most current release:

https://github.com/dev-sec/cis-dil-benchmark/actions/runs/511972343

which i believe are caused by the .rubocop.yml changes, some of which were allowed in order to align with the Inspec Style Guide recommendations

my rationale for merging this was, that it may be not a great idea to change the complete CI workflow and do a lot of syntactic code changes in one PR. We are already working on fixng the lint recommendations. :)

@schurzi , what will be the expected workflow for accepting PRs in the interim?

@rndmh3ro
Copy link
Member Author

Prior to this change the convention seemed to be that all the Checks must pass (DCO and Travis). Now that travis has been removed, there are 3 failing Checks now for the most current release:

This is not a breaking change according to semantic versioning. For the baseline itself this is not a change at all since we didn't change any of the code.

@deric4
Copy link
Member

deric4 commented Jan 26, 2021

This is not a breaking change according to semantic versioning. For the baseline itself this is not a change at all since we didn't change any of the code.

Valid point, my concern was more with how future PRs will be handled. Previously the convention was that both the DCO and Lint checks must pass before the PR was approved/merged. Would it be possible to revert the rubocop changes until the syntax updates described previously are ready so as not to cause failures in PRs in the mean time?

@rndmh3ro
Copy link
Member Author

Valid point, my concern was more with how future PRs will be handled

Yeah, you're right. This wasn't the best move, sorry. I'll fix this tomorrow. Either by fixing the linting or reverting.

@deric4
Copy link
Member

deric4 commented Jan 26, 2021

@rndmh3ro thanks! also very excited to see more use of GitHub Actions!

@schurzi
Copy link
Contributor

schurzi commented Jan 26, 2021

@deric4 sorry for the hassle. I already prepared a PR to get this in order again (#104)

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