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

Jump to bottom

core: convert util.NewSet() to sets.NewString() #8584

Merged
merged 2 commits into from Aug 25, 2021
Merged

core: convert util.NewSet() to sets.NewString() #8584

merged 2 commits into from Aug 25, 2021

Conversation

parth-gr
Copy link
Member

Converting util.NewSet() instance to use sets.NewString() instance

Closes: #8479
Signed-off-by: parth-gr paarora@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

@parth-gr
Copy link
Member Author

parth-gr commented Aug 24, 2021 •
edited

By the changes for createConfig to use string Instance
The Unit Test TestAddRemoveNode fails by throwing an error failed to get node node23 orchestration status, will try again: configmaps "rook-ceph-osd-node23-status" not found

It is because Config Map is not being returned from this call.

rook/pkg/operator/ceph/cluster/osd/status_test.go

Line 79 in 6582bac

cm, err := c.context.Clientset.CoreV1().ConfigMaps(c.clusterInfo.Namespace).Get(ctx, cmName, metav1.GetOptions{})

Need some suggestion to proceed.

BlaineEXE
BlaineEXE reviewed Aug 24, 2021
View reviewed changes
Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Seems straightforward and LGTM. One thing though: should we remove the pkg/util.set.go file/code?

@BlaineEXE
Copy link
Member

@parth-gr In response to your question, I would suggest debugging the test and stepping through the code to understand the behavior. In VisualStudio Code with the Go extension installed, there should be options to run tests hovering over test definitions. When A test is failing unexpectedly, I use debug test to investigate more deeply.

@parth-gr
Copy link
Member Author

parth-gr commented Aug 25, 2021 •
edited

Seems straightforward and LGTM. One thing though: should we remove the pkg/util.set.go file/code?

yes we are doing that.

@parth-gr In response to your question, I would suggest debugging the test and stepping through the code to understand the behavior. In VisualStudio Code with the Go extension installed, there should be options to run tests hovering over test definitions. When A test is failing unexpectedly, I use debug test to investigate more deeply.

Thanks.

parth-gr
parth-gr commented Aug 25, 2021
View reviewed changes
pkg/operator/ceph/cluster/osd/osd.go Outdated

logger.Info("start provisioning the OSDs on nodes, if needed")
nodeConfigMaps, err := c.startProvisioningOverNodes(config, errs)
if err != nil {
return err
}
statusConfigMaps.AddSet(nodeConfigMaps.Copy())
statusConfigMaps.Union(nodeConfigMaps)
Copy link
Member Author

Choose a reason for hiding this comment

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

I got the mistake, Union returns a different set which I need to store, instead of appending the list to the first instance.:)

@parth-gr
core: convert util.NewSet() to sets.NewString()
77ba1ef 
Converting util.NewSet() instance to use sets.NewString() instance

Closes: rook#8479
Signed-off-by: parth-gr <paarora@redhat.com>
@parth-gr
core: removing util.set package files
b1cfeb9 
We are no longer using package util.set
for creating instances,
and instead of that using sets.String package

Closes: rook#8479
Signed-off-by: parth-gr <paarora@redhat.com>
@travisn travisn merged commit edb8aa1 into rook:master Aug 25, 2021
@mergify mergify bot mentioned this pull request Aug 25, 2021
Merged
travisn added a commit that referenced this pull request Aug 26, 2021
@travisn
Merge pull request #8591 from rook/mergify/bp/release-1.7/pr-8584
cf4a793 
core: convert util.NewSet() to sets.NewString() (backport #8584)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BlaineEXE BlaineEXE BlaineEXE left review comments

@travisn travisn travisn approved these changes

@leseb leseb Awaiting requested review from leseb

@sp98 sp98 Awaiting requested review from sp98

@subhamkrai subhamkrai Awaiting requested review from subhamkrai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

convert util.NewSet() to sets.NewString()
3 participants
@parth-gr @BlaineEXE @travisn