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

Move storage/auth/auditLogger/theme into launcher #100

Merged
merged 8 commits into from
Jan 25, 2023
Merged

Conversation

knolleary
Copy link
Member

@knolleary knolleary commented Jan 20, 2023

Part of #99

Description

This moves the following repos into this one:

  • @flowforge/nr-audit-logger
  • @flowforge/nr-auth
  • @flowforge/nr-storage
  • @flowforge/nr-theme

I have test this locally, but experience tells me the paths around module loading at subtly different in docker/k8s land. Getting this raised now to get some eyes on it, but will want to finalise some docker-based testing before we merge.

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 flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

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

@knolleary knolleary changed the title Move storage/auth/auditLogger into launcher Move storage/auth/auditLogger/theme into launcher Jan 23, 2023
@knolleary knolleary requested a review from hardillb January 23, 2023 16:52
Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Should the icons be included twice? Once in theme/common and again in resouces

@knolleary
Copy link
Member Author

Should the icons be included twice?

Possibly not. They are in both places in the original repo, but I can't immediately see any use of them from the common folder.

May need @Steve-Mcl to confirm, but I think we can remove the images from lib/theme/common as they only ever get loaded from resources/

@Steve-Mcl
Copy link
Contributor

Should the icons be included twice?

Possibly not. They are in both places in the original repo, but I can't immediately see any use of them from the common folder.

May need @Steve-Mcl to confirm, but I think we can remove the images from lib/theme/common as they only ever get loaded from resources/

I have a vague memory of changing things around to use the resources but may not have "clean up" afterwards.

The idea was (at the time) that the theme can have own icon or fall back to common but I am unsure where it got to in the end. (sorry)

@hardillb
Copy link
Contributor

OK, I'm going to test this in a container this afternoon, assuming no other issues I'll test with/without the extra images and then merge as appropriate

package.json Outdated Show resolved Hide resolved
@hardillb
Copy link
Contributor

OK, so I removed the nr-theme line mention in the package.json dependencies and tested.

Auth, Storage and Audit logging appear to be working, but the theme has not been loaded.

@hardillb
Copy link
Contributor

Team libraries are also not working

@hardillb
Copy link
Contributor

I think I've fixed it

@knolleary
Copy link
Member Author

Okay - so I think the only remaining task is to remove the duplicate icons. I've test that locally and if it looks good, will push then get this merged.

@knolleary knolleary merged commit b0d6297 into main Jan 25, 2023
@knolleary knolleary deleted the roll-in-plugins branch January 25, 2023 14:01
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