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

Add catalogue and npmrc to App bound instances #246

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

hardillb
Copy link
Contributor

Part of FlowFuse/flowfuse#3588

Description

Allows catalogue.json URLs and .npmrc to be set for a Application bound Device

Paired with FlowFuse/flowfuse#3643

Related Issue(s)

FlowFuse/flowfuse#3588

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@hardillb hardillb marked this pull request as ready for review March 27, 2024 14:32
@hardillb hardillb linked an issue Mar 27, 2024 that may be closed by this pull request
@hardillb hardillb requested a review from Steve-Mcl March 27, 2024 14:33
Comment on lines +293 to +307
if (this.project) {
if (this.snapshot.settings?.palette?.npmrc) {
await fs.writeFile(this.files.npmrc, this.snapshot.settings.palette.npmrc)
} else {
if (existsSync(this.files.npmrc)) {
await fs.rm(this.files.npmrc)
}
}
} else if (this.application) {
if (this.settings.palette?.npmrc) {
await fs.writeFile(this.files.npmrc, this.settings.palette.npmrc)
} else {
if (existsSync(this.files.npmrc)) {
await fs.rm(this.files.npmrc)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure why snapshot is not considered when application bound?

I may need to pull and test locally to do this pair of PRs any justice.

I have another PR to review and my own PR to adjust all within the next 1h30m - i may not get this done today (sorry Ben)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Steve-Mcl I'm trying to get the Forge app side of this fixed up today, can you finish looking at this bit as well please

@Steve-Mcl Steve-Mcl self-requested a review April 10, 2024 10:36
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

Discussed the question around whether the palette catalogues/npmrc settings should be part of snapshot in Eng meeting (alongside the up-coming work around providing settings for Node-RED version (for application assigned devices) and its need to be included in the snapshot)

As it stands, this work will permit the goal of providing catalogues and npmrc but may need to be "handled" when devices are more closely aligned with instances.

Approved as is

@Steve-Mcl Steve-Mcl merged commit 10bc70b into main Apr 10, 2024
2 checks passed
@Steve-Mcl Steve-Mcl deleted the device-npmrc-catalogue branch April 10, 2024 15:36
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.

Node Catalogues & NPM configuration file for Devices
2 participants