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

feat: Support core-keeper for registry and config. #520

Merged
merged 2 commits into from
Dec 6, 2024
Merged

feat: Support core-keeper for registry and config. #520

merged 2 commits into from
Dec 6, 2024

Conversation

eaton-coreymutter
Copy link
Contributor

@eaton-coreymutter eaton-coreymutter commented Nov 8, 2024 •
edited
Loading

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/device-sdk-c/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?) (no others yet)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?) (tested a similar changeset locally, have not yet spun up a local stock-EdgeX test setup)
  • I have opened a PR for the related docs change (if not, why?) N/A

Testing Instructions

New Dependency Instructions (If applicable)

FelixTing
FelixTing requested changes Nov 22, 2024
View reviewed changes
src/c/keeper.c Outdated
{
free(keeper->topic_root);
}
if (impl)
Copy link
Member

Choose a reason for hiding this comment

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

This check is redundant because keeper is cast from impl and has been checked at the beginning.

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Outdated
} keeper_impl_t;

static int32_t edgex_keeper_client_notify(void *impl, const iot_data_t *request, const iot_data_t *pathparams, const iot_data_t *params, iot_data_t **reply);
#define CONF_PREFIX "edgex/v3/"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#define CONF_PREFIX "edgex/v3/"

unused macro

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Outdated
Comment on lines 231 to 238
err_msg = "'key' or 'value' member not found in object";
}
}
else
{
err_msg = "An element of 'response' is not a JSON object";
}
} /* End loop over responses */
Copy link
Member

Choose a reason for hiding this comment

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

break the loop when an error occurs

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Outdated

// iot_log_info(logger, "At key-root alloc, service->name %p", keeper->service->name);
keeper->key_root = calloc(URL_BUF_SIZE, 1);
snprintf(keeper->key_root, URL_BUF_SIZE-1, "edgex/v3/%s", keeper->service->name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
snprintf(keeper->key_root, URL_BUF_SIZE-1, "edgex/v3/%s", keeper->service->name);
snprintf(keeper->key_root, URL_BUF_SIZE-1, "edgex/v4/%s", keeper->service->name);

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Outdated
{
t1 = iot_time_msecs ();
*err = EDGEX_OK;
ccReady = edgex_keeper_get_tree(impl, "edgex/v3/core-common-config-bootstrapper", err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ccReady = edgex_keeper_get_tree(impl, "edgex/v3/core-common-config-bootstrapper", err);
ccReady = edgex_keeper_get_tree(impl, "edgex/v4/core-common-config-bootstrapper", err);

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Outdated
devsdk_nvpairs_free (ccReady);
}

result = edgex_keeper_get_tree(impl, "edgex/v3/core-common-config-bootstrapper/all-services", err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result = edgex_keeper_get_tree(impl, "edgex/v3/core-common-config-bootstrapper/all-services", err);
result = edgex_keeper_get_tree(impl, "edgex/v4/core-common-config-bootstrapper/all-services", err);

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Outdated
result = originalResult;

devsdk_nvpairs *privateConfig = NULL;
privateConfig = edgex_keeper_get_tree(impl, "edgex/v3/core-common-config-bootstrapper/device-services", err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
privateConfig = edgex_keeper_get_tree(impl, "edgex/v3/core-common-config-bootstrapper/device-services", err);
privateConfig = edgex_keeper_get_tree(impl, "edgex/v4/core-common-config-bootstrapper/device-services", err);

Copy link
Member

Choose a reason for hiding this comment

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

fixed in bb9db18

src/c/keeper.c Show resolved Hide resolved
src/c/keeper.c Show resolved Hide resolved
@cloudxxx8
Copy link
Member

@eaton-coreymutter do you need Felix to help address the change request?

@eaton-coreymutter
Copy link
Contributor Author

@eaton-coreymutter do you need Felix to help address the change request?

I should be able to fix it. I missed it because we migrated to common config but have not done much editing of the common configs.

@FelixTing FelixTing mentioned this pull request Dec 6, 2024
Open
@FelixTing
Copy link
Member

@eaton-coreymutter I have pushed a commit to address the change requests. Could you take a look when you get a chance, please?

@cloudxxx8
Copy link
Member

@FelixTing please merge this PR first to proceede the continous testing
@eaton-coreymutter please feel free leave any comments in this PR and we will open another PR to address them

@FelixTing FelixTing mentioned this pull request Dec 6, 2024
Merged
5 tasks
@FelixTing FelixTing merged commit 62fc64c into edgexfoundry:main Dec 6, 2024
3 checks passed
@eaton-coreymutter
Copy link
Contributor Author

The changes all seem good.

Are we changing the Keeper keys from /v3 to /v4 even though the APIs are remaining /v3? One consequence of that is that fielded products will need config migration. Then again that's probably a problem unique to Eaton, as early adopters we had that with /v2 to /v3.

@cloudxxx8
Copy link
Member

@eaton-coreymutter although the api version is v3, the key prefix is the software version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelixTing FelixTing FelixTing approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@eaton-coreymutter @cloudxxx8 @FelixTing