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.

Sign up for GitHub

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

Jump to bottom

Refactor to Facilitate Testability #21

Merged
merged 94 commits into from
Nov 21, 2022
Merged

Refactor to Facilitate Testability #21

merged 94 commits into from
Nov 21, 2022

Conversation

mfleader
Copy link
Member

@mfleader mfleader commented Nov 9, 2022 •
edited
Loading

Changes introduced with this PR

  1. Refactor internal sub-packages by incorporating more dependency injections to facilitate the addition of unit tests and mocked interfaces for unit tests to reach the organization's target 60% line coverage.
  2. Adds golangci-lint configuration and pre-commit configuration to enforce quality, style, and linting
  3. Adds limgo configuration to calculate line coverage

By contributing to this repository, I agree to the contribution guidelines.

@mfleader mfleader added the enhancement New feature or request label Nov 9, 2022
@mfleader mfleader self-assigned this Nov 9, 2022
@mfleader mfleader requested a review from dustinblack November 15, 2022 17:37
@sandrobonazzola sandrobonazzola self-requested a review November 16, 2022 07:33
@mfleader mfleader marked this pull request as ready for review November 16, 2022 13:08
ghost
ghost approved these changes Nov 16, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Minor nit-picks

.github/workflows/limgo.yaml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
internal/docker/client.go Show resolved Hide resolved
internal/images/images_test.go Outdated

import (
"fmt"
"github.com/arcalot/arcaflow-plugin-image-builder/internal/dto"
Copy link

Choose a reason for hiding this comment

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

Still needs to be replaced.

internal/requirements/basic.go Outdated Show resolved Hide resolved
limgo_cov.md Outdated Show resolved Hide resolved
@mfleader mfleader removed the request for review from jaredoconnell November 17, 2022 18:35
portante
portante reviewed Nov 21, 2022
View reviewed changes
Copy link

@portante portante left a comment

Choose a reason for hiding this comment

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

This PR is tough to review. Perhaps there is no way around it, but with 54 files change, some of them being "lift-n-shift", and 93 commits, it presents a bit of a challenge.

If you are interested, I'd be happy to help with using git to restructure the commits in the PR for review.

@portante portante merged commit 80f1f6a into arcalot:main Nov 21, 2022
@mfleader mfleader deleted the refactor-engelmi-2 branch November 22, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@portante portante portante left review comments

@dustinblack dustinblack dustinblack approved these changes

@sandrobonazzola sandrobonazzola sandrobonazzola approved these changes

@engelmi engelmi Awaiting requested review from engelmi

@jdowni000 jdowni000 Awaiting requested review from jdowni000

Assignees

@mfleader mfleader

Labels
enhancement New feature or request
Projects
No open projects
Arcaflow
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mfleader @sandrobonazzola @portante @dustinblack @engelmi