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

First pass at file api #275

Merged
merged 23 commits into from
Aug 28, 2024
Merged

First pass at file api #275

merged 23 commits into from
Aug 28, 2024

Conversation

hardillb
Copy link
Contributor

fixes #273

Description

Implement API from FlowFuse/flowfuse#4357

Related Issue(s)

#273

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

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

@hardillb hardillb self-assigned this Aug 13, 2024
@Steve-Mcl
Copy link
Contributor

Quick one - as discussed in eng meeting.

the regex on the routes block valid file names and paths that for example contain a space.

Also consolidate the regex into the single variable to make
changes easier
Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Can you check your linter settings? lib/files/index.js has a bunch of minor formatting issues that eslint is flagging.

Some other comments along the way.

lib/files/index.js Outdated Show resolved Hide resolved
lib/files/index.js Outdated Show resolved Hide resolved
lib/files/index.js Outdated Show resolved Hide resolved
@hardillb
Copy link
Contributor Author

hmm, lint settings should be checked into the project and it' passes when I run npm run lint

@knolleary
Copy link
Member

passes when I run npm run lint

The npm task is incorrectly configured:

https://github.com/FlowFuse/nr-launcher/blob/main/package.json#L23-L24

It is linting lib/*.js - only files in lib. Should be lib/**/*.js to ensure sub directories are linted. In doing so, it finds a bunch of linting issues in other files.

lib/files/index.js Outdated Show resolved Hide resolved
@hardillb hardillb marked this pull request as ready for review August 27, 2024 09:53
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.

Code reviewed and pulled locally by myself and @joepavitt - all goodness and working.

Approving (with blessing) and acknowledge there are no new tests for the new files api and runtimesettings changes) for which I will add a follow up chore issue.

@Steve-Mcl Steve-Mcl requested review from knolleary and removed request for knolleary August 28, 2024 16:35
@hardillb hardillb dismissed knolleary’s stale review August 28, 2024 16:38

Because he's on holiday

@hardillb hardillb merged commit 0c05f78 into main Aug 28, 2024
5 checks passed
@hardillb hardillb deleted the file-api branch August 28, 2024 16:38
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.

Expose API for file tasks
3 participants