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

Refactor step structure #30

Merged
merged 20 commits into from
Aug 25, 2021
Merged

Refactor step structure #30

merged 20 commits into from
Aug 25, 2021

Conversation

ofalvai
Copy link
Contributor

@ofalvai ofalvai commented Aug 18, 2021

Checklist

Context

Refactoring step structure, updating go-utils and handling its breaking changes.

Changes

  • Remove deprecated input apk_path_pattern. This was marked to be removed on 20 August 2019. ¯_(ツ)_/¯
  • Refactor step structure, move most of the logic from main.go to step.go
  • Add some new tests

Investigation details

Decisions

main.go Outdated Show resolved Hide resolved
step/step.go Show resolved Hide resolved
step/step.go Outdated Show resolved Hide resolved
step/step.go Outdated Show resolved Hide resolved
if err != nil {
return Config{}, nil
}
stepconf.Print(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

we had this TODO in activate-ssh-key: TODO: log.Infof(stepconf.toString(input)), we could solve it with this PR

step/step.go Outdated Show resolved Hide resolved
@godrei
Copy link
Contributor

godrei commented Aug 18, 2021

I would consider the refactoring done if we can write unit tests like:

  • When AppTyp is apk Then assemble task is executed
  • When XYZ module is specified, Then XYZ build command gets executed

step/step.go Outdated Show resolved Hide resolved
step/step.go Outdated Show resolved Hide resolved
@ofalvai ofalvai marked this pull request as ready for review August 23, 2021 07:56
Copy link
Contributor

@Bence1001 Bence1001 left a comment

Choose a reason for hiding this comment

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

I've got a few minor comments, but the rest LGTM!

step/step.go Outdated Show resolved Hide resolved
step/step.go Show resolved Hide resolved
step/step.go Outdated Show resolved Hide resolved
Bence1001
Bence1001 previously approved these changes Aug 24, 2021
Copy link
Contributor

@Bence1001 Bence1001 left a comment

Choose a reason for hiding this comment

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

LGTM!

godrei
godrei previously approved these changes Aug 24, 2021
@ofalvai ofalvai merged commit 6930079 into master Aug 25, 2021
@ofalvai ofalvai deleted the step-1214-refactor-step branch August 25, 2021 07:37
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