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

Changes required for parallel build jobs #24

Merged
merged 5 commits into from
Jun 16, 2020
Merged

Conversation

amdprophet
Copy link
Member

Signed-off-by: Justin Kolberg [email protected]

@amdprophet amdprophet requested a review from palourde June 16, 2020 21:28
Copy link
Contributor

@palourde palourde left a comment

Choose a reason for hiding this comment

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

👌

Copy link
Contributor

@echlebek echlebek 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 clean up the functions that don't return errors. I'd say they are complex enough to deserve tests as well?

cmd/circleci-logs/fetch.go Outdated Show resolved Hide resolved
cmd/circleci-logs/fetchall.go Outdated Show resolved Hide resolved
@amdprophet
Copy link
Member Author

@echlebek This code is already being used in CI so I'll open an issue to write tests and add this repository to CircleCI. I'll implement the other two suggestions now.

@echlebek
Copy link
Contributor

I think it's really important that this code can be maintained. I'd prefer if tests were added before accepting this PR even if CI isn't set up, as these types of things tend to languish. Could you add even a smoke test? +1 on filing an issue for CI setup though. CC @portertech, we may want to add this repository to the zenhub board.

@amdprophet
Copy link
Member Author

amdprophet commented Jun 16, 2020

CI issue: #25

Signed-off-by: Justin Kolberg <[email protected]>
Signed-off-by: Justin Kolberg <[email protected]>
@amdprophet
Copy link
Member Author

@echlebek added - and they even found an issue!

@amdprophet amdprophet merged commit 2a51484 into master Jun 16, 2020
@amdprophet amdprophet deleted the parallel-jobs branch June 16, 2020 22:07
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