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

[Vue Rewrite] Fix CI Jobs #1816

Merged
merged 19 commits into from
Jun 9, 2022
Merged

[Vue Rewrite] Fix CI Jobs #1816

merged 19 commits into from
Jun 9, 2022

Conversation

devlinjunker
Copy link
Contributor

@devlinjunker devlinjunker commented Jun 7, 2022 •
edited
Loading

Start of getting a hold of the Vue Rewrite

  • This fixes update CI Stuff in Vue rewrite #748
    • Note: as part of conversations with @SMillerDev, we removed the npm test that did echo 'TODO' -- so we expect the Frontend tests CI to fail now
  • Ensure runs on Node v16
    • added a bunch of libraries to package.json This is most of the actual substantial changes in this PR
  • Passes eslint and stylelint checks
    • Most of this PR is just indentation/style changes so that these will pass
    • needs more cleanup here.. but it is passing for now
  • Added brief update to CHANGELOG.MD for rewrite (necessary for CI)
    • TBD if 19.x.x is correct... could be pushed if this loses momentum

See devlinjunker#1 for full work history and comments

Visual

vue-rewrite working

Up Next:

devlinjunker added 17 commits June 6, 2022 21:38
@devlinjunker
attempt to fix linting
bf6bd55 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
working linting/stylelint
6dcdd57 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
fix npm build
69f8b74 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
working with node 16 and npm build
6a83890 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
fix linting
d88e541 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
add @babel/eslint-parser
06d0722 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
remove babel-eslint
81a6be9 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
use node 16 in lint
0ffc1a3 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
proper syntax
215fb95 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
fix syntax
fb016e5 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
update version
e2c8df9 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
attempt to undo indentation changes
c037bfe 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
revert indentation on css and update stylelint rule for now (for simp…
6029073 
…ler PR)

Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
revert indentation changes in vue/js files and update eslint to pass …
4a23bd0 
…this (for now)

Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
try to cleanup disabled rules
6612bf1 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
remove last disabled rules in js files
6abc6ce 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
add basic changelog
ef0a05c 
Signed-off-by: Devlin Junker <[email protected]>
SMillerDev
SMillerDev reviewed Jun 7, 2022
View reviewed changes
package.json Outdated
@@ -10,7 +10,8 @@
"lint": "eslint --ext .js,.vue src",
"lint:fix": "eslint --ext .js,.vue src --fix",
"stylelint": "stylelint **/*.css **/*.scss **/*.vue",
"stylelint:fix": "stylelint **/*.css **/*.scss **/*.vue --fix"
"stylelint:fix": "stylelint **/*.css **/*.scss **/*.vue --fix",
"test": "echo 'TODO'"
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably shouldn't be here

Copy link
Contributor Author

@devlinjunker devlinjunker Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm pretty sure that this needed to be here for the CI checks to pass ( see the error here https://github.com/nextcloud/news/runs/6544192521?check_suite_focus=true )

in another PR we can try to add mocha/karma unit tests that can be run here, but for now it is just a TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or do you mean we should leave this ci check failing until they are properly implemented?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if it's not doing anything CI shouldn't be checking it iyam.

Copy link
Contributor Author

@devlinjunker devlinjunker Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

hmm.. I could just disable this check I suppose...

But I think I like the idea of leaving it failing until the tests are written so it kinda blocks the rewrite PR until then

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me

Copy link
Contributor Author

@devlinjunker devlinjunker Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this will fail that CI job though (Frontend tests), so we will have to merge this PR with ignoring that check

@devlinjunker
update calendar-js to 3.0
9455743 
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker
Copy link
Contributor Author

devlinjunker commented Jun 9, 2022 •
edited
Loading

any other thoughts?

should we merge this so I can create a PR for adding typescript?

@SMillerDev SMillerDev merged commit 40d9c35 into nextcloud:vue-rewrite Jun 9, 2022
@Grotax
Copy link
Member

Grotax commented Jun 9, 2022

Thank you for the work 😊👍

@devlinjunker devlinjunker mentioned this pull request Jun 17, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SMillerDev SMillerDev SMillerDev 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
@devlinjunker @Grotax @SMillerDev