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

update assets with new default config #809

Merged
merged 5 commits into from
Nov 5, 2020
Merged

Conversation

butonic
Copy link
Member

@butonic butonic commented Nov 5, 2020

Currently, I have to give consent after login because the generated identifier-registration.yaml lacks the trusted: yes config option.

I regenerated the assets with make clean generate build but now I no longer have any web ui.

Instead I now have this in the log:

2020-11-05T17:07:32+01:00 FTL Could not open index template error="file does not exist" service=konnectd
process [konnectd] exited with code: 1

Where is the make target to update the assets with the nice new branding @LukasHirt ?

I also made the config/identifier-registration.yaml just point to the ../assets so we don't forget to update the config in two places next time.

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@update-docs
Copy link

update-docs bot commented Nov 5, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic butonic requested review from IljaN and LukasHirt November 5, 2020 16:06
@LukasHirt
Copy link
Collaborator

LukasHirt commented Nov 5, 2020

Where is the make target to update the assets with the nice new branding @LukasHirt ?

You need to run yarn build before generate. That's building the UI into assets folder. Maybe we should make it part of the make generate? Or some other command which would combine those two?

@butonic
Copy link
Member Author

butonic commented Nov 5, 2020

yarn build
yarn run v1.22.4
warning package.json: "dependencies" has dependency "eslint" with range "^6.6.0" that collides with a dependency in "devDependencies" of the same name with version ">=5"
warning package.json: "dependencies" has dependency "eslint-plugin-react" with range "7.18.0" that collides with a dependency in "devDependencies" of the same name with version "^7.10.0"
$ node scripts/build.js && rm -f build/service-worker.js
internal/modules/cjs/loader.js:968
  throw err;
  ^

Error: Cannot find module 'dotenv-expand'
Require stack:
- /home/jfd/Repositories/ocis/konnectd/ui_config/env.js
- /home/jfd/Repositories/ocis/konnectd/scripts/build.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:965:15)
    at Function.Module._load (internal/modules/cjs/loader.js:841:27)
    at Module.require (internal/modules/cjs/loader.js:1025:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at /home/jfd/Repositories/ocis/konnectd/ui_config/env.js:35:5
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/jfd/Repositories/ocis/konnectd/ui_config/env.js:33:13)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/jfd/Repositories/ocis/konnectd/ui_config/env.js',
    '/home/jfd/Repositories/ocis/konnectd/scripts/build.js'
  ]
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@butonic
Copy link
Member Author

butonic commented Nov 5, 2020

doing yarn install

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
Copy link
Member Author

butonic commented Nov 5, 2020

@LukasHirt @IljaN I updated the makefile to include the necessary yarn magic. It will execute yarn build when the assets/identifier/index.html does not exist and also do a yarn_install when the node_modules folder is missing

Copy link
Member

@IljaN IljaN left a comment

Choose a reason for hiding this comment

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

LGTM 👯

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@@ -84,12 +86,21 @@ lint:
generate: assets
go generate $(GENERATE)

.PHONY: assets
assets:
# TODO find a docker container with go and yarn so we can properly build assets in ci
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to introduce package caching as well (pre-fetched and bundled in CI container)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes ... but if I ask @micbar about that again he might want to hurt me ... we should schedule it for the next sprint ...

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a dedicated container. Just split it in two commands. Run yarn build in the nodejs container and go generate in the golang container. That is how we do it in all the other extensions.

@butonic butonic merged commit 47a877c into owncloud:master Nov 5, 2020
@butonic butonic deleted the fix-konnectd branch November 5, 2020 22:27
ownclouders pushed a commit that referenced this pull request Nov 5, 2020
Merge: c124102 3d1fa00
Author: Jörn Friedrich Dreyer <[email protected]>
Date:   Thu Nov 5 23:27:20 2020 +0100

    Merge pull request #809 from butonic/fix-konnectd

    update assets with new default config
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.

5 participants