Skip to content

Conversation

@sromerotech
Copy link

I've added the vhost parameter so you can customize it. I'll appreciate any feedback on this PR. Thanks in advance.

@AlvaroVega
Copy link
Member

Thaks for your contribution @sergiorb !
Could you add some documentation about new parameter IOTA_AMQP_VHOST in https://github.com/telefonicaid/iotagent-json/blob/master/docs/installationguide.md#amqp-binding-configuration?

@AlvaroVega
Copy link
Member

LGTM
A twin PR for iota-ul is expected

Co-authored-by: Fermín Galán Márquez <[email protected]>
Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

@sromerotech
Copy link
Author

Hello @AlvaroVega @fgalan and sorry for the delay. I've updated the branch with some test that needs a specially set-up RabbitMQ to run due to the creation of the virtual host is not possible from the amqplib library. May you check that out? I'll replicate the changes in Iotagent-ul repo as soon as you validate this one. Thanks in advance.

@@ -0,0 +1,223 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

instead of -test3.js it would be better a more meaningful suffix, according to the kind of test cases included in this file.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe test/unit/amqpBinding-vhost-test.js ?

Copy link
Author

Choose a reason for hiding this comment

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

Nice! I just followed what I found there. Name it as you like.

sromerotech and others added 2 commits March 22, 2021 13:24
@@ -0,0 +1,223 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

test/unit/amqpBinding-vhost-test.js.js -> test/unit/amqpBinding-vhost-test.js

Too much "js" at the end ;)

@fgalan
Copy link
Member

fgalan commented Mar 23, 2021

LGTM. Thanks!

We will merge once you can address the following, please:

With regards to README.md, maybe we will do some refactor of the documentation of your contribution but with regards this PR is fine.

Comment on lines 1 to 7
- Fix: avoid raise mongo alarm when a measure is not maching a group configuration
- Upgrade NodeJS version from 10 to 12 in Dockerfile due to Node 10 End-of-Life
- Set Nodejs 12 as minimum version in packages.json (effectively removing Nodev10 from supported versions)
- Add list of environment variables which can be protected by Docker Secrets
- Fix: avoid raise mongo alarm when a measure is not maching a group configuration
- Fix: Set 60 seconds for default mqtt keepalive option (#413)
- Upgrade NodeJS version from 10 to 12 in Dockerfile due to Node 10 End-of-Life
- Set Nodejs 12 as minimum version in packages.json (effectively removing Nodev10 from supported versions)
- Add: Support of multimeasure for MQTT and AMQP transport (#462)
- Add: virtual host configuration added to AMQP transport (config.amqp.vhost and IOTA_AMQP_VHOST env var)
- Add list of environment variables which can be protected by Docker Secrets
Copy link
Member

@fgalan fgalan Mar 26, 2021

Choose a reason for hiding this comment

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

Not sure if the conflict in CHANGES_NEXT_RELEASE has been correctly solved...

CHANGES_NEXT_RELEASE at this PR branch should contain the current entries in master (i.e. https://github.com/telefonicaid/iotagent-json/blob/master/CHANGES_NEXT_RELEASE) plus the line associated to this PR (i.e. - Add: virtual host configuration added to AMQP transport (config.amqp.vhost and IOTA_AMQP_VHOST env var)).

@sromerotech
Copy link
Author

Hello @fgalan and thank you for your feedback. I've updated the CHANGES_NEXT_RELEASE file as requested and also updated README.md with a more concise note about the RabbitMQ configuration. Notice that rabbitmq:management is necessary for the test to pass.
I also have pushed these changes to the telefonicaid/iotagent-ul#462 PR.

@fgalan
Copy link
Member

fgalan commented Mar 26, 2021

Could you have a look to the tests run in GitActions, please? It seems many of them are failing...

@sromerotech
Copy link
Author

sromerotech commented Mar 29, 2021

Could you have a look to the tests run in GitActions, please? It seems many of them are failing...

I've been working on them and I cannot find a way to mount the definitions file as volume without breaking the pipeline. It seems that Git checkout step tries to delete all files, being unable to delete de mounted definition. I've opened a PR in rabbitmq-github-action in order to remove rabbitmq service and add that action as a step before running the test.

On the other hand, I've run master-branch's tests locally and they fail too, mainly due to ContextBroker issues. Could you take a look on that?

Thanks.

@fgalan
Copy link
Member

fgalan commented Mar 29, 2021

On the other hand, I've ran master-branch's tests locally and they fail too, mainly due to ContextBroker issues. Could you take a look on that?

Nos sure what do you mean... according to github, test are passing ok in master at the present moment:

imagen

@fgalan
Copy link
Member

fgalan commented Apr 9, 2021

Nos sure what do you mean... according to github, test are passing ok in master at the present moment:

After careful examination, this is not fully true... The tests were ok in master when I look at it (the screenshot is correct) but I have re-run the tests in master and they have failed. This may be caused due to two reasons:

  • Changes not in the IOTA code itself but in a "open dependency" (i.e. dependency in packages.json that points to a branch instead of a version number of commit). At the present moment, I think iotagent-node-lib is the only dependency of this kind
  • Changes in GitHub Actions (although I don't figure out which one could be...)

We are analyzing it.

CC: @AlvaroVega

@fgalan
Copy link
Member

fgalan commented Apr 13, 2021

Nos sure what do you mean... according to github, test are passing ok in master at the present moment:

After careful examination, this is not fully true... The tests were ok in master when I look at it (the screenshot is correct) but I have re-run the tests in master and they have failed. This may be caused due to two reasons:

  • Changes not in the IOTA code itself but in a "open dependency" (i.e. dependency in packages.json that points to a branch instead of a version number of commit). At the present moment, I think iotagent-node-lib is the only dependency of this kind
  • Changes in GitHub Actions (although I don't figure out which one could be...)

We are analyzing it.

CC: @AlvaroVega

At the end, it was due to iotagent-node-lib. Some changes on that library make some test to be "unaligned". It has been already fixed in master (by PR #550).

Thus, could you please update this PR brach con master. After that I guess that all your changes in ci.yml in this PR are not needed and can be reverted. Remember also to remove packages-lock.json from the PR. Thanks!

@sromerotech sromerotech closed this by deleting the head repository May 11, 2024
@fgalan
Copy link
Member

fgalan commented May 13, 2024

@sromerotech could you explain why this PR has been closed, please? Do you plan to re-create it? Thanks!

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.

3 participants