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

A bit of code refactoring #805

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

A bit of code refactoring #805

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f72b649
Skipping "IsOutdatedConfig" because I think is useless, it only check…
Nov 14, 2024
4d99530
removed unused error
Nov 14, 2024
1f50336
removed the merger struct, it did nothing there
Nov 14, 2024
3fc49e8
removed unused function
Nov 15, 2024
dcf0135
removed commented and unused code
Nov 15, 2024
9fc5404
return of IsOutdatedConfig function
Feb 4, 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
13 changes: 6 additions & 7 deletions pkg/autosync/autosync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,19 @@ import (
// in case where user specific values that are defined in granted.yml's `templateValues` are not available.
// this is done so that users are aware of required keys when granted's credential-process is used through the AWS CLI.
func Run(ctx context.Context, interactive bool) {
if registry.IsOutdatedConfig() {
clio.Warn("Outdated Profile Registry Configuration. Use `granted registry migrate` to update your configuration.")

clio.Warn("Skipping Profile Registry sync.")

return
}

registries, err := registry.GetProfileRegistries(interactive)
if err != nil {
clio.Debugf("unable to load granted config file with err %s", err.Error())
return
}

if len(registries) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

registry.IsOutdatedConfig() checks a different field in the Granted config to the one in registry.GetProfileRegistries().

IsOutdatedConfig checks if the length of ProfileRegistryURLS is > 0, whereas GetProfileRegistries uses the ProfileRegistry field.

The change you have made here will cause valid profile registry configuration to return the error. Could you please restore the previous behaviour?

In this instance, I'd propose just restoring the registry.IsOutdatedConfig function and restoring the call to the function. The code there was working in production, so there is no need to change it IMO.

clio.Warn("Outdated Profile Registry Configuration. Use `granted registry migrate` to update your configuration.")
clio.Warn("Skipping Profile Registry sync.")
return
}

// check if registry has been configured or not.
// should skip registry sync if no profile registry.
if len(registries) == 0 {
Expand Down
8 changes: 0 additions & 8 deletions pkg/autosync/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@ import (
"github.com/common-fate/granted/pkg/granted/registry"
)

type RegistrySyncError struct {
err error
}

func (e *RegistrySyncError) Error() string {
return fmt.Sprintf("error syncing profile registry with err: %s", e.err.Error())
}

func runSync(ctx context.Context, rc RegistrySyncConfig, interactive bool) error {
clio.Info("Syncing Profile Registries")
err := registry.SyncProfileRegistries(ctx, interactive)
Expand Down
2 changes: 1 addition & 1 deletion pkg/granted/awsmerge/merge_from_registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func autoGeneratedMsg() string {
// [granted_registry_start test]
// // ... <generated profiles>
// [granted_registry_end test]
func (m Merger) WithRegistry(src *ini.File, dst *ini.File, opts RegistryOpts) (*ini.File, error) {
func WithRegistry(src *ini.File, dst *ini.File, opts RegistryOpts) (*ini.File, error) {
namespace := opts.Name

// create a temporary ini structure, so that we leave 'dst' untouched if we run into
Expand Down
3 changes: 1 addition & 2 deletions pkg/granted/awsmerge/merge_from_registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,7 @@ key1 = value2
t.Fatal(err)
}

m := Merger{}
out, err := m.WithRegistry(src, dst, tt.args.opts)
out, err := WithRegistry(src, dst, tt.args.opts)
if (err != nil) != tt.wantErr {
t.Fatalf("Merger.WithRegistry() error = %v, wantErr %v", err, tt.wantErr)
}
Expand Down
6 changes: 0 additions & 6 deletions pkg/granted/awsmerge/merger.go
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion pkg/granted/awsmerge/remove_registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
// [granted_registry_start test]
// // ... <generated profiles>
// [granted_registry_end test]
func (m Merger) RemoveRegistry(src *ini.File, name string) {
func RemoveRegistry(src *ini.File, name string) {
// replace any existing generated profiles which match the name of the registry
existing := getGrantedGeneratedSections(src, name)
for _, p := range existing {
Expand Down
12 changes: 4 additions & 8 deletions pkg/granted/registry/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ var AddCommand = cli.Command{
return err
}

m := awsmerge.Merger{}

merged, err := m.WithRegistry(src, dst, awsmerge.RegistryOpts{
merged, err := awsmerge.WithRegistry(src, dst, awsmerge.RegistryOpts{
Name: name,
PrefixAllProfiles: prefixAllProfiles,
PrefixDuplicateProfiles: prefixDuplicateProfiles,
Expand Down Expand Up @@ -140,7 +138,7 @@ var AddCommand = cli.Command{
registryConfig.PrefixDuplicateProfiles = true

// try and merge again
merged, err = m.WithRegistry(src, dst, awsmerge.RegistryOpts{
merged, err = awsmerge.WithRegistry(src, dst, awsmerge.RegistryOpts{
Name: name,
PrefixAllProfiles: prefixAllProfiles,
PrefixDuplicateProfiles: true,
Expand Down Expand Up @@ -184,9 +182,7 @@ var AddCommand = cli.Command{
return err
}

m := awsmerge.Merger{}

merged, err := m.WithRegistry(src, dst, awsmerge.RegistryOpts{
merged, err := awsmerge.WithRegistry(src, dst, awsmerge.RegistryOpts{
Name: name,
PrefixAllProfiles: prefixAllProfiles,
PrefixDuplicateProfiles: prefixDuplicateProfiles,
Expand Down Expand Up @@ -216,7 +212,7 @@ var AddCommand = cli.Command{
registryConfig.PrefixDuplicateProfiles = true

// try and merge again
merged, err = m.WithRegistry(src, dst, awsmerge.RegistryOpts{
merged, err = awsmerge.WithRegistry(src, dst, awsmerge.RegistryOpts{
Name: name,
PrefixAllProfiles: prefixAllProfiles,
PrefixDuplicateProfiles: true,
Expand Down
14 changes: 0 additions & 14 deletions pkg/granted/registry/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,3 @@ var MigrateCommand = cli.Command{
return nil
},
}

func IsOutdatedConfig() bool {
gConf, err := grantedConfig.Load()
if err != nil {
clio.Debug(err.Error())
return true
}

if len(gConf.ProfileRegistryURLS) > 0 {
return true
}

return false
}
4 changes: 1 addition & 3 deletions pkg/granted/registry/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,12 @@ var RemoveCommand = cli.Command{
return err
}

m := awsmerge.Merger{}

configFile, awsConfigPath, err := loadAWSConfigFile()
if err != nil {
return err
}

m.RemoveRegistry(configFile, selectedRegistry.Name)
awsmerge.RemoveRegistry(configFile, selectedRegistry.Name)

err = configFile.SaveTo(awsConfigPath)
if err != nil {
Expand Down
6 changes: 2 additions & 4 deletions pkg/granted/registry/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,13 @@ func SyncProfileRegistries(ctx context.Context, interactive bool) error {
return nil
}

m := awsmerge.Merger{}

for _, r := range registries {
src, err := r.Registry.AWSProfiles(ctx, interactive)
if err != nil {
return fmt.Errorf("error retrieving AWS profiles for registry %s: %w", r.Config.Name, err)
}

merged, err := m.WithRegistry(src, configFile, awsmerge.RegistryOpts{
merged, err := awsmerge.WithRegistry(src, configFile, awsmerge.RegistryOpts{
Name: r.Config.Name,
PrefixAllProfiles: r.Config.PrefixAllProfiles,
PrefixDuplicateProfiles: r.Config.PrefixDuplicateProfiles,
Expand Down Expand Up @@ -92,7 +90,7 @@ func SyncProfileRegistries(ctx context.Context, interactive bool) error {
}

// try and merge again
merged, err = m.WithRegistry(src, configFile, awsmerge.RegistryOpts{
merged, err = awsmerge.WithRegistry(src, configFile, awsmerge.RegistryOpts{
Name: r.Config.Name,
PrefixAllProfiles: r.Config.PrefixAllProfiles,
PrefixDuplicateProfiles: true,
Expand Down