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

integration_test: Create helpers to initialize tests with custom indexes #598

Merged
merged 4 commits into from
May 3, 2020

Conversation

chriskim06
Copy link
Member

Two changes here:

  • WithIndex -> WithDefaultIndex
  • added WithCustomIndex function that does the cloning from the default index

I wasn't really sure if WithCustomIndex should call initializeIndex if it hasn't been initialized yet. It would clean up the code so that people don't have to call WithDefaultIndex().WithCustomIndex("foo") but I was worried that the cloning of krew-index was a little too hidden this way.

If I'm overthinking it let me know and I can change it up so that WithCustomIndex initializes krew-index if it hasn't been initialized yet.

Fixes #565
Related issue: #566

/area multi-index

@k8s-ci-robot k8s-ci-robot added area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 30, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 30, 2020
@ahmetb
Copy link
Member

ahmetb commented Apr 30, 2020

I think this mostly LGTM, and I wouldn't mind if WithCustomIndex() failed with error if default index is not initialized.

Also WithCustomIndex() name isn't the best IMO, it doesn't indicate that it's created by mirroring the default index. I'm open to other proposals.

@chriskim06
Copy link
Member Author

How about WithCustomIndexFromDefault? Its kind of a wordy/long function name but I think it captures the intent behind it (new custom index "from" default index).

I thought I'd just throw out the most obvious correction to what I had to get your opinion on that, but I'll keep giving this some thought throughout the day.

@ahmetb
Copy link
Member

ahmetb commented Apr 30, 2020

We (maintainers) read the tests so it's fine. it also now makes sense why WithDefaultIndex is there :)

@chriskim06
Copy link
Member Author

Ok I'll change it to that in a bit

@chriskim06
Copy link
Member Author

chriskim06 commented Apr 30, 2020

Here's something a little strange. The integration tests took 75 seconds to run here, but the action that got launched in my fork only took 36 seconds.

Also forgot to mention but I haven't seen it take that long locally either.

@ahmetb
Copy link
Member

ahmetb commented Apr 30, 2020

Could be anything. If it's one off, nothing to worry, could be throttling (by repo, by org, global). But if it repeats consistently after merged, then we should probably investigate.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor nits.

Comment on lines 324 to 327
enabled = true
}
}
return enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
enabled = true
}
}
return enabled
return true
}
}
return false

it.initializeIndex()
return it
}

// WithCustomIndexFromDefault initializes a new index by cloning the default index. WithDefaultIndex needs
// to be called before this function.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth explaining why we want this testutil in the comment. This is another good reason to move this logic into a function, because it gives us a good place where we can document this.

@ahmetb
Copy link
Member

ahmetb commented May 3, 2020

/lgtm
/approve
Thank you. 🙏

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, chriskim06

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2020
@k8s-ci-robot k8s-ci-robot merged commit 045a61f into kubernetes-sigs:master May 3, 2020
@chriskim06 chriskim06 deleted the custom-index-helper branch June 11, 2020 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

integration_test: Create helpers to initialize tests with custom indexes
4 participants