Skip to content

Enable to filter suggested applications when you add an application from controlplane #3320

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
pipecd-bot merged 3 commits into from
Feb 28, 2022
Merged

Enable to filter suggested applications when you add an application from controlplane #3320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70b73d2
Enable to filter applications that specific piped will handle
knanao Feb 28, 2022
dfe6445
Fix test
knanao Feb 28, 2022
c7a9842
Update docs/content/en/docs-dev/operator-manual/piped/configuration-r…
knanao Feb 28, 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
1 change: 1 addition & 0 deletions docs/content/en/docs-dev/operator-manual/piped/configuration-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ spec:
| eventWatcher | [EventWatcher](/docs/operator-manual/piped/configuration-reference/#eventwatcher) | Optional Event watcher settings. | No |
| secretManagement | [SecretManagement](/docs/operator-manual/piped/configuration-reference/#secretmanagement) | The using secret management method. | No |
| notifications | [Notifications](/docs/operator-manual/piped/configuration-reference/#notifications) | Sending notifications to Slack, Webhook... | No |
| appSelector | map[string]string | List of labels to filter all applications this piped will handle. Currently, it is only be used to filter the applications suggested for adding from the control plane. | No |
Copy link
Member

@khanhtc1202 khanhtc1202 Feb 28, 2022

Choose a reason for hiding this comment

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

not really related to this pull but I just realized that this is List of ... so should we change the name as appSelectors?

Copy link
Member Author

@knanao knanao Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is based on k8s selector.
But it seems good to combine appSelectors with labels. wdyt?

Copy link
Member

@khanhtc1202 khanhtc1202 Feb 28, 2022

Choose a reason for hiding this comment

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

Then it will be appLabelsSelector right? 😄

Copy link
Member

@khanhtc1202 khanhtc1202 Feb 28, 2022

Choose a reason for hiding this comment

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

on the second thought, I guess labels is unnecessary, so I think we should keep it as is. Sorry for the interruption 🙏

Copy link
Member Author

@knanao knanao Feb 28, 2022

Choose a reason for hiding this comment

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

Umm.., It took me a while to think appSelector is okay.
Do you feel a bit strange?

Copy link
Member

@khanhtc1202 khanhtc1202 Feb 28, 2022

Choose a reason for hiding this comment

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

Yep, so lets keep it as is, thank you 🙌


## Git

Expand Down
11 changes: 10 additions & 1 deletion pkg/app/piped/appconfigreporter/appconfigreporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,10 @@ func (r *Reporter) isSynced(appInfo *model.ApplicationInfo, app *model.Applicati
// findUnregisteredApps finds out unregistered application info in the given git repository.
// The file name must be default name in order to be recognized as an Application config.
func (r *Reporter) findUnregisteredApps(repoPath, repoID string) ([]*model.ApplicationInfo, error) {
apps := r.applicationLister.List()
var (
apps = r.applicationLister.List()
selector = r.config.AppSelector
)
// Create a map to determine the app is registered by GitPath.
registeredAppPaths := make(map[string]struct{}, len(apps))
for _, app := range apps {
Expand Down Expand Up @@ -355,6 +358,12 @@ func (r *Reporter) findUnregisteredApps(repoPath, repoID string) ([]*model.Appli
// Continue reading so that it can return apps as much as possible.
return nil
}

// Filter the apps by appSelector if appSelector set.
if len(selector) != 0 && !appInfo.ContainLabels(selector) {
return nil
}

out = append(out, appInfo)
return nil
})
Expand Down
91 changes: 89 additions & 2 deletions pkg/app/piped/appconfigreporter/appconfigreporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ func TestReporter_findUnregisteredApps(t *testing.T) {
"path/to/repo-1/app-1/app.pipecd.yaml": &fstest.MapFile{Data: []byte("")},
},
logger: zap.NewNop(),
config: &config.PipedSpec{
PipedID: "piped-1",
},
},
args: args{
repoPath: "invalid",
Expand All @@ -198,6 +201,9 @@ func TestReporter_findUnregisteredApps(t *testing.T) {
"path/to/repo-1/app-1/app.pipecd.yaml": &fstest.MapFile{Data: []byte("")},
},
logger: zap.NewNop(),
config: &config.PipedSpec{
PipedID: "piped-1",
},
},
args: args{
repoPath: "path/to/repo-1",
Expand All @@ -216,6 +222,9 @@ func TestReporter_findUnregisteredApps(t *testing.T) {
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/app.pipecd.yaml": &fstest.MapFile{Data: []byte("invalid-text")},
},
config: &config.PipedSpec{
PipedID: "piped-1",
},
logger: zap.NewNop(),
},
args: args{
Expand All @@ -229,7 +238,9 @@ func TestReporter_findUnregisteredApps(t *testing.T) {
{
name: "valid app config that is unregistered",
reporter: &Reporter{
config: &config.PipedSpec{PipedID: "piped-1"},
config: &config.PipedSpec{
PipedID: "piped-1",
},
applicationLister: &fakeApplicationLister{},
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/app.pipecd.yaml": &fstest.MapFile{Data: []byte(`
Expand Down Expand Up @@ -262,7 +273,9 @@ spec:
{
name: "valid app config that name isn't default",
reporter: &Reporter{
config: &config.PipedSpec{PipedID: "piped-1"},
config: &config.PipedSpec{
PipedID: "piped-1",
},
applicationLister: &fakeApplicationLister{},
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/dev.pipecd.yaml": &fstest.MapFile{Data: []byte(`
Expand Down Expand Up @@ -292,6 +305,80 @@ spec:
},
wantErr: false,
},
{
name: "filtered by appSelector",
reporter: &Reporter{
config: &config.PipedSpec{
PipedID: "piped-1",
AppSelector: map[string]string{
"env": "test",
},
},
applicationLister: &fakeApplicationLister{},
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/app.pipecd.yaml": &fstest.MapFile{Data: []byte(`
apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
name: app-1
labels:
key-1: value-1
env: dev
`)},
},
logger: zap.NewNop(),
},
args: args{
repoPath: "path/to/repo-1",
repoID: "repo-1",
registeredAppPaths: map[string]string{},
},
want: []*model.ApplicationInfo{},
wantErr: false,
},
{
name: "match labels with appSelector",
reporter: &Reporter{
config: &config.PipedSpec{
PipedID: "piped-1",
AppSelector: map[string]string{
"env": "test",
},
},
applicationLister: &fakeApplicationLister{},
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/dev.pipecd.yaml": &fstest.MapFile{Data: []byte(`
apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
name: app-1
labels:
key-1: value-1
env: test
`)},
},
logger: zap.NewNop(),
},
args: args{
repoPath: "path/to/repo-1",
repoID: "repo-1",
registeredAppPaths: map[string]string{},
},
want: []*model.ApplicationInfo{
{
Name: "app-1",
Labels: map[string]string{
"key-1": "value-1",
"env": "test",
},
RepoId: "repo-1",
Path: "app-1",
ConfigFilename: "dev.pipecd.yaml",
PipedId: "piped-1",
},
},
wantErr: false,
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
Expand Down