Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

add tests for releasesync #1089

Merged
merged 2 commits into from
May 22, 2018

Conversation

ncabatoff
Copy link
Contributor

Here are some tests for releasesync to address #1037. Beyond the additional tests, I:

  • killed off some unreferenced symbols, made some helper methods private
  • defined the interface Releaser, made ReleaseChangeSync depend on it
  • expanded godoc comments
  • added some calls to errors.Wrap
  • added a dependency on go-cmp

Questions:

  • Is the ignoring of errors in calls to addDeletedReleasesToSync/addExistingReleasesToSync intentional?
  • Why do we do the 'git pull' after having computed what work to do, rather than before?

I'd be happy to continue down this path and add more tests if my approach is satisfactory to the maintainers, though I think sync() is the only other method in this package that's particularly wanting testing.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

LGTM. I admit I have not tried to fully understand the code as it stands. But some test coverage is very welcome!

// and returns them organised by namespace and chart release name.
// map[namespace] = []releaseFhr.
func (rs *ReleaseChangeSync) getCustomResources(
ifClient ifclientset.Clientset,

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@squaremo
Copy link
Member

You'll need to rebase on master and rerun dep, since there was another PR that modified Gopkg.toml.

You may as well squash a couple of the fixup commits while you're there ;-)

@ncabatoff
Copy link
Contributor Author

Ok, will rebase/dep/squash as you say. I had a couple of questions (re potential bugs) about the code in my PR; should I open separate issues for them, or ask on slack?

@squaremo
Copy link
Member

Is the ignoring of errors in calls to addDeletedReleasesToSync/addExistingReleasesToSync intentional?

Probably not; I think it's reasonable to leave a FIXME comment there and move on for now.

Why do we do the 'git pull' after having computed what work to do, rather than before?

Yes that is odd.

Forensics (git blame) indicates that it was part of a fairly large set of changes regarding how git repos were used. Given that, and the alpha status of the helm operator, it's plausible that this particular code is logically wrong but (mostly) works. I reckon this one is worth an issue.

@ncabatoff ncabatoff force-pushed the 1037-add-tests-for-releasesync branch from 843f9b9 to 32b2a21 Compare May 21, 2018 17:17
@ncabatoff
Copy link
Contributor Author

Ok, added the fixme comment. I did the squash/etc, but please have another look because I'm pretty new to multi-user git workflows and it's possible I messed something up. I'll open an issue for the late pull.

@ncabatoff
Copy link
Contributor Author

#1092 for the odd pull timing.

@squaremo squaremo merged commit 721605e into fluxcd:master May 22, 2018
@squaremo
Copy link
Member

Brill 🌟 thanks @ncabatoff

@ncabatoff ncabatoff deleted the 1037-add-tests-for-releasesync branch May 22, 2018 13:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants