Skip to content

refactor(config): destructify project-level getter and setters in package config #138

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

Merged
mwbrooks merged 7 commits into from
Jun 25, 2025
Merged

refactor(config): destructify project-level getter and setters in package config #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
31fed26
refactor(config): destructify config.GetProjectDirPath(fs, os)
mwbrooks Jun 14, 2025
3cb7a62
refactor(config): destructify config.ProjectConfigJSONFileExists(fs, …
mwbrooks Jun 14, 2025
7ac2b4a
refactor(config): destructify config.WriteProjectConfigFile(ctx, fs, …
mwbrooks Jun 14, 2025
5ea82cc
refactor(config): destructify config.ReadProjectConfigFile(ctx, fs, os)
mwbrooks Jun 14, 2025
d3a47b5
refactor(config): destructify config.SetManifestSource(ctx, fs, os, s…
mwbrooks Jun 16, 2025
2a4bd2a
Merge branch 'main' into mwbrooks-destructify-config
mwbrooks Jun 23, 2025
e442e0f
Merge branch 'main' into mwbrooks-destructify-config-project-p2
mwbrooks Jun 25, 2025
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
1 change: 1 addition & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"slackdeps",
"slackerror",
"slackhq",
"slackmock",
"slacktrace",
"tabletests",
"testutil"
Expand Down
2 changes: 1 addition & 1 deletion cmd/app/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty
return nil
}

if err := clients.Config.ProjectConfig.SetManifestSource(ctx, config.ManifestSourceRemote); err != nil {
if err := config.SetManifestSource(ctx, clients.Fs, clients.Os, config.ManifestSourceRemote); err != nil {
// Log the error to the verbose output
clients.IO.PrintDebug(ctx, "Error setting manifest source in project-level config: %s", err)
// Display a user-friendly error with a workaround
Expand Down
10 changes: 5 additions & 5 deletions cmd/app/link_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ func Test_Apps_Link(t *testing.T) {
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
// Set manifest source to project to trigger confirmation prompt
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.ManifestSourceLocal); err != nil {
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}
// Accept manifest source confirmation prompt
Expand Down Expand Up @@ -509,7 +509,7 @@ func Test_Apps_Link(t *testing.T) {
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
// Set manifest source to project to trigger confirmation prompt
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.ManifestSourceLocal); err != nil {
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}
// Decline manifest source confirmation prompt
Expand Down Expand Up @@ -547,7 +547,7 @@ func Test_Apps_Link(t *testing.T) {
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
// Set manifest source to local
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.ManifestSourceLocal); err != nil {
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}
// Mock manifest for Run-on-Slack app
Expand Down Expand Up @@ -624,7 +624,7 @@ func Test_Apps_Link(t *testing.T) {
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
// Set manifest source to local
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.ManifestSourceLocal); err != nil {
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceLocal); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}
// Mock manifest for Run-on-Slack app
Expand Down Expand Up @@ -760,7 +760,7 @@ func setupAppLinkCommandMocks(t *testing.T, ctx context.Context, cm *shared.Clie
require.FailNow(t, fmt.Sprintf("Failed to create the hooks file in the memory-based file system: %s", err))
}

if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.ManifestSourceRemote); err != nil {
if err := config.SetManifestSource(ctx, cm.Fs, cm.Os, config.ManifestSourceRemote); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}

Expand Down
3 changes: 2 additions & 1 deletion cmd/doctor/check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"strings"
"time"

"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/deputil"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/pkg/version"
Expand Down Expand Up @@ -132,7 +133,7 @@ func checkProjectConfig(ctx context.Context, clients *shared.ClientFactory) Sect
Label: "Configurations",
Value: "your project's CLI settings",
}
projectConfig, err := clients.Config.ProjectConfig.ReadProjectConfigFile(ctx)
projectConfig, err := config.ReadProjectConfigFile(ctx, clients.Fs, clients.Os)
if err != nil {
if slackerror.ToSlackError(err).Code != slackerror.ErrInvalidAppDirectory {
section.Errors = append(section.Errors, *slackerror.ToSlackError(err))
Expand Down
11 changes: 8 additions & 3 deletions cmd/doctor/check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ import (
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/slackmock"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -144,10 +146,13 @@ func TestDoctorCheckProjectConfig(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
pcm := &config.ProjectConfigMock{}
pcm.On("ReadProjectConfigFile", mock.Anything).Return(tt.projectConfig, nil)
clientsMock.Config.ProjectConfig = pcm
clients := shared.NewClientFactory(clientsMock.MockClientFactory())

slackmock.CreateProject(t, ctx, clients.Fs, clients.Os, slackdeps.MockWorkingDirectory)

_, err := config.WriteProjectConfigFile(ctx, clients.Fs, clients.Os, tt.projectConfig)
require.NoError(t, err)

expected := Section{
Label: "Configurations",
Value: "your project's CLI settings",
Expand Down
14 changes: 9 additions & 5 deletions cmd/doctor/doctor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ import (
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/slackmock"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -57,14 +59,16 @@ func TestDoctorCommand(t *testing.T) {
clientsMock.Auth.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything).Return("api.slack.com")
clientsMock.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{}, nil)
clientsMock.AddDefaultMocks()
pcm := &config.ProjectConfigMock{}
pcm.On("ReadProjectConfigFile", mock.Anything).Return(config.ProjectConfig{

slackmock.CreateProject(t, ctx, clientsMock.Fs, clientsMock.Os, slackdeps.MockWorkingDirectory)
_, err := config.WriteProjectConfigFile(ctx, clientsMock.Fs, clientsMock.Os, config.ProjectConfig{
ProjectID: expectedProjectID,
Manifest: &config.ManifestConfig{
Source: config.ManifestSourceLocal.String(),
},
}, nil)
clientsMock.Config.ProjectConfig = pcm
})
require.NoError(t, err)

scm := &config.SystemConfigMock{}
scm.On("GetSurveyConfig", mock.Anything, mock.Anything).Return(config.SurveyConfig{}, nil)
scm.On("SetSurveyConfig", mock.Anything, mock.Anything, mock.Anything).Return(nil)
Expand All @@ -88,7 +92,7 @@ func TestDoctorCommand(t *testing.T) {

cmd := NewDoctorCommand(clients)
testutil.MockCmdIO(clients.IO, cmd)
err := cmd.ExecuteContext(ctx)
err = cmd.ExecuteContext(ctx)
require.NoError(t, err)

report, err := performChecks(ctx, clients)
Expand Down
2 changes: 1 addition & 1 deletion internal/app/app_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (ac *AppClient) CleanUp() {

// if there are no tracked apps anymore and no config file, remove the .slack folder.
// otherwise remove .slack/apps*.json files that contain no apps.
if ac.apps.IsEmpty() && !ac.config.ProjectConfig.ProjectConfigJSONFileExists(wd) {
if ac.apps.IsEmpty() && !config.ProjectConfigJSONFileExists(ac.fs, ac.os, wd) {
var deployedAppsJSONFilePath = filepath.Join(wd, deployedAppsFilename)
var dotSlackFolder = filepath.Dir(deployedAppsJSONFilePath)
_ = ac.fs.RemoveAll(dotSlackFolder)
Expand Down
2 changes: 1 addition & 1 deletion internal/app/app_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ func TestAppClient_CleanupSlackFolder(t *testing.T) {
assert.True(t, ac.apps.IsEmpty(), "an unexpected app was found")

require.NoError(t, err)
assert.False(t, ac.config.ProjectConfig.ProjectConfigJSONFileExists(wd),
assert.False(t, config.ProjectConfigJSONFileExists(ac.fs, ac.os, wd),
"an unexpected config was found")

dotSlackFolder := filepath.Dir(pathToAppsJSON)
Expand Down
2 changes: 1 addition & 1 deletion internal/config/experiments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (c *Config) LoadExperiments(
}
printDebug(ctx, fmt.Sprintf("active flag experiments: %s", experiments))
// Load from project config file
projectConfig, err := c.ProjectConfig.ReadProjectConfigFile(ctx)
projectConfig, err := ReadProjectConfigFile(ctx, c.fs, c.os)
if err != nil && slackerror.ToSlackError(err).Code != slackerror.ErrInvalidAppDirectory {
printDebug(ctx, fmt.Sprintf("failed to parse project-level config file: %s", err))
} else if err == nil {
Expand Down
42 changes: 19 additions & 23 deletions internal/config/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,8 @@ type ProjectConfigManager interface {
GetProjectID(ctx context.Context) (string, error)
SetProjectID(ctx context.Context, projectID string) (string, error)
GetManifestSource(ctx context.Context) (ManifestSource, error)
SetManifestSource(ctx context.Context, source ManifestSource) error
GetSurveyConfig(ctx context.Context, name string) (SurveyConfig, error)
SetSurveyConfig(ctx context.Context, name string, surveyConfig SurveyConfig) error
ReadProjectConfigFile(ctx context.Context) (ProjectConfig, error)
WriteProjectConfigFile(ctx context.Context, projectConfig ProjectConfig) (string, error)
ProjectConfigJSONFileExists(projectDirPath string) bool

Cache() cache.Cacher
}
Expand Down Expand Up @@ -119,7 +115,7 @@ func (c *ProjectConfig) GetProjectID(ctx context.Context) (string, error) {
span, ctx = opentracing.StartSpanFromContext(ctx, "GetProjectID")
defer span.Finish()

var projectConfig, err = c.ReadProjectConfigFile(ctx)
var projectConfig, err = ReadProjectConfigFile(ctx, c.fs, c.os)
if err != nil {
return "", err
}
Expand All @@ -133,14 +129,14 @@ func (c *ProjectConfig) SetProjectID(ctx context.Context, projectID string) (str
span, _ = opentracing.StartSpanFromContext(ctx, "SetProjectID")
defer span.Finish()

var projectConfig, err = c.ReadProjectConfigFile(ctx)
var projectConfig, err = ReadProjectConfigFile(ctx, c.fs, c.os)
if err != nil {
return "", err
}

projectConfig.ProjectID = projectID

_, err = c.WriteProjectConfigFile(ctx, projectConfig)
_, err = WriteProjectConfigFile(ctx, c.fs, c.os, projectConfig)
if err != nil {
return "", err
}
Expand All @@ -154,7 +150,7 @@ func (c *ProjectConfig) GetManifestSource(ctx context.Context) (ManifestSource,
span, ctx = opentracing.StartSpanFromContext(ctx, "GetManifestSource")
defer span.Finish()

var projectConfig, err = c.ReadProjectConfigFile(ctx)
var projectConfig, err = ReadProjectConfigFile(ctx, c.fs, c.os)
if err != nil {
return "", err
}
Expand All @@ -175,18 +171,18 @@ func (c *ProjectConfig) GetManifestSource(ctx context.Context) (ManifestSource,
}

// SetManifestSource saves the manifest source preference for the project
func (c *ProjectConfig) SetManifestSource(ctx context.Context, source ManifestSource) error {
func SetManifestSource(ctx context.Context, fs afero.Fs, os types.Os, source ManifestSource) error {
span, ctx := opentracing.StartSpanFromContext(ctx, "SetManifestSource")
defer span.Finish()
projectConfig, err := c.ReadProjectConfigFile(ctx)
projectConfig, err := ReadProjectConfigFile(ctx, fs, os)
if err != nil {
return err
}
if projectConfig.Manifest == nil {
projectConfig.Manifest = &ManifestConfig{}
}
projectConfig.Manifest.Source = source.String()
_, err = c.WriteProjectConfigFile(ctx, projectConfig)
_, err = WriteProjectConfigFile(ctx, fs, os, projectConfig)
if err != nil {
return err
}
Expand All @@ -199,7 +195,7 @@ func (c *ProjectConfig) GetSurveyConfig(ctx context.Context, name string) (Surve
span, ctx = opentracing.StartSpanFromContext(ctx, "GetSurveyConfig")
defer span.Finish()

var projectConfig, err = c.ReadProjectConfigFile(ctx)
var projectConfig, err = ReadProjectConfigFile(ctx, c.fs, c.os)
if err != nil {
return SurveyConfig{}, err
}
Expand All @@ -218,7 +214,7 @@ func (c *ProjectConfig) SetSurveyConfig(ctx context.Context, name string, survey
span, ctx = opentracing.StartSpanFromContext(ctx, "SetSurveyConfig")
defer span.Finish()

var projectConfig, err = c.ReadProjectConfigFile(ctx)
var projectConfig, err = ReadProjectConfigFile(ctx, c.fs, c.os)
if err != nil {
return err
}
Expand All @@ -228,7 +224,7 @@ func (c *ProjectConfig) SetSurveyConfig(ctx context.Context, name string, survey
CompletedAt: surveyConfig.CompletedAt,
}

_, err = c.WriteProjectConfigFile(ctx, projectConfig)
_, err = WriteProjectConfigFile(ctx, c.fs, c.os, projectConfig)
if err != nil {
return err
}
Expand All @@ -237,24 +233,24 @@ func (c *ProjectConfig) SetSurveyConfig(ctx context.Context, name string, survey
}

// ReadProjectConfigFile reads the project-level config.json file
func (c *ProjectConfig) ReadProjectConfigFile(ctx context.Context) (ProjectConfig, error) {
func ReadProjectConfigFile(ctx context.Context, fs afero.Fs, os types.Os) (ProjectConfig, error) {
var span opentracing.Span
span, _ = opentracing.StartSpanFromContext(ctx, "ReadProjectConfigFile")
defer span.Finish()

var projectConfig ProjectConfig

projectDirPath, err := GetProjectDirPath(c.fs, c.os)
projectDirPath, err := GetProjectDirPath(fs, os)
if err != nil {
return projectConfig, err
}
Comment on lines +243 to 246
Copy link
Member

@zimeg zimeg Jun 24, 2025

Choose a reason for hiding this comment

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

👁️‍🗨️ This might be because I'm used to the before version of this code, but to me this:

projectConfig, err := clients.Config.ProjectConfig.ReadProjectConfigFile(ctx)

Hid the magic of how the project path was found. I realize now we're doing the same thing with arguments shared instead, but it wasn't obvious how fs and os were used to read the configuration to me...

projectConfig, err := config.ReadProjectConfigFile(ctx, clients.Fs, clients.Os)

I do think this is good improvement for testing and using this package overall and it's not so confusing IRL but I wanted to share an initial thought here!

Copy link
Member Author

@mwbrooks mwbrooks Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree @zimeg, this function has always felt magical to me as well.

Behind the scenes, it's using the current working directory as the project path, but that's bad practice imo. This kind of logic makes it difficult for us to support running commands in sub-directories and it confusing to read as a maintainer.

We should definitely improve it in the future!


if !c.ProjectConfigJSONFileExists(projectDirPath) {
if !ProjectConfigJSONFileExists(fs, os, projectDirPath) {
return projectConfig, nil
}

var projectConfigFilePath = GetProjectConfigJSONFilePath(projectDirPath)
projectConfigFileBytes, err := afero.ReadFile(c.fs, projectConfigFilePath)
projectConfigFileBytes, err := afero.ReadFile(fs, projectConfigFilePath)
if err != nil {
return projectConfig, err
}
Expand All @@ -274,7 +270,7 @@ func (c *ProjectConfig) ReadProjectConfigFile(ctx context.Context) (ProjectConfi
}

// WriteProjectConfigFile writes the project-level config.json file
func (c *ProjectConfig) WriteProjectConfigFile(ctx context.Context, projectConfig ProjectConfig) (string, error) {
func WriteProjectConfigFile(ctx context.Context, fs afero.Fs, os types.Os, projectConfig ProjectConfig) (string, error) {
var span opentracing.Span
span, _ = opentracing.StartSpanFromContext(ctx, "WriteProjectConfigFile")
defer span.Finish()
Expand All @@ -284,13 +280,13 @@ func (c *ProjectConfig) WriteProjectConfigFile(ctx context.Context, projectConfi
return "", err
}

projectDirPath, err := GetProjectDirPath(c.fs, c.os)
projectDirPath, err := GetProjectDirPath(fs, os)
if err != nil {
return "", err
}

projectConfigFilePath := GetProjectConfigJSONFilePath(projectDirPath)
err = afero.WriteFile(c.fs, projectConfigFilePath, projectConfigBytes, 0600)
err = afero.WriteFile(fs, projectConfigFilePath, projectConfigBytes, 0644)
if err != nil {
return "", err
}
Expand All @@ -299,9 +295,9 @@ func (c *ProjectConfig) WriteProjectConfigFile(ctx context.Context, projectConfi
}

// ProjectConfigJSONFileExists returns true if the .slack/config.json file exists
func (c *ProjectConfig) ProjectConfigJSONFileExists(projectDirPath string) bool {
func ProjectConfigJSONFileExists(fs afero.Fs, os types.Os, projectDirPath string) bool {
var projectConfigFilePath = GetProjectConfigJSONFilePath(projectDirPath)
_, err := c.fs.Stat(projectConfigFilePath)
_, err := fs.Stat(projectConfigFilePath)
return !os.IsNotExist(err)
}

Expand Down
20 changes: 0 additions & 20 deletions internal/config/project_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ func (m *ProjectConfigMock) GetManifestSource(ctx context.Context) (ManifestSour
return args.Get(0).(ManifestSource), args.Error(1)
}

func (m *ProjectConfigMock) SetManifestSource(ctx context.Context, source ManifestSource) error {
args := m.Called(ctx, source)
return args.Error(0)
}

func (m *ProjectConfigMock) GetSurveyConfig(ctx context.Context, id string) (SurveyConfig, error) {
args := m.Called(ctx, id)
return args.Get(0).(SurveyConfig), args.Error(1)
Expand All @@ -68,21 +63,6 @@ func (m *ProjectConfigMock) SetSurveyConfig(ctx context.Context, id string, surv
return args.Error(0)
}

func (m *ProjectConfigMock) ReadProjectConfigFile(ctx context.Context) (ProjectConfig, error) {
args := m.Called(ctx)
return args.Get(0).(ProjectConfig), args.Error(1)
}

func (m *ProjectConfigMock) WriteProjectConfigFile(ctx context.Context, projectConfig ProjectConfig) (string, error) {
args := m.Called(ctx, projectConfig)
return args.String(0), args.Error(1)
}

func (m *ProjectConfigMock) ProjectConfigJSONFileExists(projectDirPath string) bool {
args := m.Called(projectDirPath)
return args.Bool(0)
}

// Cache returns a persistent mock cache
func (m *ProjectConfigMock) Cache() cache.Cacher {
args := m.Called()
Expand Down
Loading
Loading