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

[chore]: dont set path for go in validate #6273

Merged
merged 9 commits into from
Oct 25, 2022
Merged

[chore]: dont set path for go in validate #6273

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
16e2ede
fix: dont set path for go in validate
sakshi1215 Oct 11, 2022
c1b9e9d
test
sakshi1215 Oct 11, 2022
a95bb9c
fix
sakshi1215 Oct 11, 2022
7a7ddf6
add changelog entry
sakshi1215 Oct 11, 2022
ae77541
Update CHANGELOG.md
sakshi1215 Oct 12, 2022
90d7ed1
Update CHANGELOG.md
sakshi1215 Oct 12, 2022
e150cfd
Update cmd/builder/internal/builder/config.go
sakshi1215 Oct 13, 2022
9ea4b78
fix
sakshi1215 Oct 20, 2022
3c9091c
fix
sakshi1215 Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/builder/internal/builder/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ func NewDefaultConfig() Config {
}
}

// Validate checks whether the current configuration is valid
func (c *Config) Validate() error {
// ValidateAndSetGoPath checks whether the current configuration is valid and sets go path
func (c *Config) ValidateAndSetGoPath() error {
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer to split this function into two:

  • SetGoPath
  • Validate

The current callers of Validate will then need to call both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @jpkrohling

if !c.SkipCompilation || !c.SkipGetModules {
// #nosec G204
if _, err := exec.Command(c.Distribution.Go, "env").CombinedOutput(); err != nil {
Expand Down
10 changes: 5 additions & 5 deletions cmd/builder/internal/builder/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,14 @@ func TestInvalidModule(t *testing.T) {
}

for _, test := range configurations {
assert.True(t, errors.Is(test.cfg.Validate(), test.err))
assert.True(t, errors.Is(test.cfg.ValidateAndSetGoPath(), test.err))
}
}

func TestNewDefaultConfig(t *testing.T) {
cfg := NewDefaultConfig()
require.NoError(t, cfg.ParseModules())
require.NoError(t, cfg.Validate())
require.NoError(t, cfg.ValidateAndSetGoPath())
}

func TestNewBuiltinConfig(t *testing.T) {
Expand All @@ -155,7 +155,7 @@ func TestNewBuiltinConfig(t *testing.T) {

require.NoError(t, k.UnmarshalWithConf("", &cfg, koanf.UnmarshalConf{Tag: "mapstructure"}))
assert.NoError(t, cfg.ParseModules())
assert.NoError(t, cfg.Validate())
assert.NoError(t, cfg.ValidateAndSetGoPath())

// Unlike the config initialized in NewDefaultConfig(), we expect
// the builtin default to be practically useful, so there must be
Expand All @@ -174,14 +174,14 @@ func TestSkipGoValidation(t *testing.T) {
SkipCompilation: true,
SkipGetModules: true,
}
assert.NoError(t, cfg.Validate())
assert.NoError(t, cfg.ValidateAndSetGoPath())
}

func TestSkipGoInitialization(t *testing.T) {
cfg := Config{
SkipCompilation: true,
SkipGetModules: true,
}
assert.NoError(t, cfg.Validate())
assert.NoError(t, cfg.ValidateAndSetGoPath())
assert.Zero(t, cfg.Distribution.Go)
}
2 changes: 1 addition & 1 deletion cmd/builder/internal/builder/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func TestGenerateAndCompileDefault(t *testing.T) {
// we override this version, otherwise this would break during releases
cfg.Distribution.OtelColVersion = "0.52.0"

assert.NoError(t, cfg.Validate())
assert.NoError(t, cfg.ValidateAndSetGoPath())
require.NoError(t, GenerateAndCompile(cfg))

// Sleep for 1 second to make sure all processes using the files are completed
Expand Down
2 changes: 1 addition & 1 deletion cmd/builder/internal/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ configuration is provided, ocb will generate a default Collector.
if err := initConfig(cmd.Flags()); err != nil {
return err
}
if err := cfg.Validate(); err != nil {
if err := cfg.ValidateAndSetGoPath(); err != nil {
return fmt.Errorf("invalid configuration: %w", err)
}

Expand Down