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

Exclude default serviceaccount on backup #195

Merged
merged 1 commit into from Mar 10, 2022

Conversation

superseb
Copy link
Contributor

#165

Root cause is described in #165 (comment)

@superseb superseb requested a review from a team February 18, 2022 23:02
for _, resObj := range resourceObjectsList.Items {
metadata := resObj.Object["metadata"].(map[string]interface{})
name := metadata["name"].(string)
nameMatched, err := regexp.MatchString(filter.ExcludeResourceNameRegexp, name)
Copy link

Choose a reason for hiding this comment

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

If I'm understanding this correctly, this line should return true if the string in the variable name starts or ends with default, but I tried this in the go playground using the pattern ^default$ and got false instead. I do get that behavior if I use the pattern ^default|default$, but I see we are using the ^<string>$ pattern to match other things already so am I missing something? https://go.dev/play/p/7EtHr0IZKfR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exclude is for service accounts called default, the playground shows comparing token names with the regex. We are excluding the default service accounts created in each namespace and they are named default. Let me know if I need to add some clarification in the issue or code.

Choose a reason for hiding this comment

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

Okay, makes sense now. I think the comment you linked to is good, this is just my first time reviewing the backup-restore-operator.

Copy link

@pennyscissors pennyscissors left a comment

Choose a reason for hiding this comment

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

lgtm

@superseb superseb merged commit ff53d9b into rancher:master Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants