Skip to content

Filter subscribe on --product. - #149

Merged
mpejcoch merged 6 commits into
masterfrom
issue_127_filtering
Jun 21, 2021
Merged

Filter subscribe on --product. #149
mpejcoch merged 6 commits into
masterfrom
issue_127_filtering

Conversation

@havardf

@havardf havardf commented Jun 18, 2021

Copy link
Copy Markdown
Collaborator

Refactor to allow for integration tests on mms commandline tool. Makefile with test_integration rule.

Commands now get used directly, that is, without calling a funcation that returns a function. Since there was no closures there, I don't think the added indirection was needed?

Filter by product on both productReceive and command callback function.

…s on mms commandline tool. Makefile with test_integration rule.
@havardf
havardf requested a review from mpejcoch June 18, 2021 07:44
@havardf
havardf marked this pull request as draft June 18, 2021 07:48
@havardf

havardf commented Jun 18, 2021

Copy link
Copy Markdown
Collaborator Author

I forgot to fix api-key generation on integration_tests. Set as draft until thats fixed.

@mpejcoch

Copy link
Copy Markdown
Contributor

Refactor to allow for integration tests on mms commandline tool. Makefile with test_integration rule.

Commands now get used directly, that is, without calling a funcation that returns a function. Since there was no closures there, I don't think the added indirection was needed?

No, I don't think so either, we just assumed that it had some hidden purpose that we can't see yet. This is easier to read 👍🏻

Good idea with the integration test.

Comment thread Makefile Outdated
go-plantuml generate -rd . -o go-mms.puml

test_integration: build_mmsd
$(eval MMSD_KEY=$(shell ./mmsd keys --gen|awk 'NR==1{print $$3}'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This populates the default state.db with keys, maybe some tmp state.db would be better?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point, that would be better. I guess then, you would check in that file, and start mmsd with ./mmsd -w ./test_dir, or something like that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is the DB in the repo now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see. It's for tests, right? If so, that may clutter the initial config space of a new instance of mmsd. It would probably be better to put it in a folder for test files, or generate it for the test as I think @mpejcoch suggests?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was what I meant, yes. Generate it on start, but give it a -w <temp_test_dir> so that it doesn't interact with whatever might be present from before. It is perhaps less of a problem when the testing is going to be done in a fresh container, so far we need to count on this being built and deployed on the PPI nodes too.

I see an advantage of having the DB in the repo though, which is that it would catch possible regressions towards the DB structure, as the state.db is something that users will keep after upgrading to a new mmsd version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But a populated db will cause conflicts if it is run from a git clone, so it's a bit risky to version control it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having a committed db isn't in itself a problem in my opinion, but it shouldn't be located at the default location in the repo, but rather in a resource folder for tests that mmsd is pointed to. Then there is no risk of conflicting with a live db.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Aha! I misunderstood Martin to begin with. But it simplifies things, to have a static test db in repo, to avoid having to dynamically set api keys for the integrations tests etc.

So, maybe its ok to create have in ./test_data/state.db?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds like a good solution.

@havardf
havardf marked this pull request as ready for review June 20, 2021 15:24
@vkbo

vkbo commented Jun 21, 2021

Copy link
Copy Markdown
Contributor

Since there was no closures there, I don't think the added indirection was needed?

No, I don't think so either, we just assumed that it had some hidden purpose that we can't see yet. This is easier to read 👍🏻

Yeah, we didn't understand why but also didn't try to change it :)

@mpejcoch
mpejcoch merged commit dcb80ea into master Jun 21, 2021
@mpejcoch
mpejcoch deleted the issue_127_filtering branch June 21, 2021 13:32
@mortenwh mortenwh added this to the Full project milestone Aug 4, 2021
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.

4 participants