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

init: Option to write config file to stdout. #47

Merged
merged 7 commits into from
Mar 31, 2023

Conversation

winder
Copy link
Contributor

@winder winder commented Mar 30, 2023

Summary

While creating a docker container, I found that it could be used like docker run algorand/conduit init to create a config file. The problem is that output is written to a file.

This changes the behavior of conduit init. Previously it would create a directory named data containing the config file, now it writes the config file to stdout. To get the previous behavior you would use the command conduit init -d data.

Test Plan

Unit tests are updated to cover the new behavior.

@winder winder requested a review from a team March 30, 2023 13:51
@winder winder self-assigned this Mar 30, 2023
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #47 (5a49e51) into master (e74cca4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   66.23%   66.23%           
=======================================
  Files          33       33           
  Lines        1869     1869           
=======================================
  Hits         1238     1238           
  Misses        566      566           
  Partials       65       65           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

Looking good to me and works on my local machine👍
Minor question below

cmd/conduit/internal/initialize/init.go Outdated Show resolved Hide resolved
cmd/conduit/internal/initialize/init.go Outdated Show resolved Hide resolved
@winder winder requested a review from a team March 30, 2023 19:14
Copy link
Contributor

@shiqizng shiqizng left a comment

Choose a reason for hiding this comment

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

LGTM. minor suggestions.

cmd/conduit/internal/initialize/init.go Outdated Show resolved Hide resolved
fmt.Printf("\nOnce the config file is updated, start Conduit with:\n")
fmt.Printf(" ./conduit -d %s\n", path)
// If a data dir is created, print some additional help.
if path != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it may be better to use configWriter==nil check here; one fewer var to track. path is getting updated at line 58 and it only happens when configWriter == nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

configWriter is set to the file on line 74. I agree that the logic is strange, maybe I should just refactor to avoid the strange logic.

@winder
Copy link
Contributor Author

winder commented Mar 30, 2023

@shiqizng @algochoi I updated the code to simplify the things you pointed out.

@winder winder merged commit 37b9b6a into algorand:master Mar 31, 2023
@winder winder deleted the will/init-to-file branch March 31, 2023 12:01
tzaffi pushed a commit to tzaffi/conduit that referenced this pull request May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants