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

validate release actions support mulit os #197

Merged
merged 24 commits into from
Apr 7, 2023

Conversation

z7658329
Copy link
Member

@z7658329 z7658329 commented Feb 11, 2023

@z7658329 z7658329 changed the title feat/validate support mulit os validate release actions support mulit os Feb 11, 2023
COUNT=$(find . -type f -size +900k | wc -l)
if [[ $COUNT -ne 0 ]]; then
find . -type f -size +900k
echo "The package shouldn't include file larger than 900kb, but get $COUNT"
echo "The package shouldn't include file larger than 900kb, but get $COUNT" && exit 1
fi
COUNT=$(find . -type f | perl -lne 'print if -B' | grep -v *.txt | wc -l)
Copy link
Member

Choose a reason for hiding this comment

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

need a better way to search binary files(and exclude some files marked in properties)

no we can't exit here,but we should search the key word in action log manually

@@ -250,3 +270,4 @@ jobs:
fail-fast: false
matrix:
java_version: [ '8','11' ]
os: [ubuntu-latest, macos-latest] # TODO: support windows-latest or other os in future
Copy link
Member

Choose a reason for hiding this comment

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

perfer not to use comment after the line(use a separate line)

and seems miss centos here?

Copy link
Member Author

Choose a reason for hiding this comment

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

i try to find centos, It doesn't seem to be found

Copy link
Contributor

Choose a reason for hiding this comment

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

seems github does not support CentOS, the supported runners: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

I think we can mark "TODO: support CentOS" now.
how to support CentOS: https://zhuanlan.zhihu.com/p/344367359

@imbajin imbajin linked an issue Feb 13, 2023 that may be closed by this pull request
1 task
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

can we move validate-release.sh from hugegraph-server to hugegraph-doc?

@@ -180,6 +182,7 @@ jobs:
echo "test hubble"
cd ./*hubble*${{ inputs.release_version }} || exit
cat conf/hugegraph-hubble.properties && bin/start-hubble.sh
# TODO: need stop the server here
Copy link
Contributor

Choose a reason for hiding this comment

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

we can also add # kill the HugeGraphServer process by jps at line 154

Copy link
Member Author

Choose a reason for hiding this comment

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

we can also add # kill the HugeGraphServer process by jps at line 154

seems should stop server after toolchain test

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's difficult to find a time to stop, we can close the old one before starting server.
image

fi
COUNT=$(find . -type f | perl -lne 'print if -B' | grep -v *.txt | wc -l)
if [[ $COUNT -ne 0 ]]; then
find . -type f | perl -lne 'print if -B'
# due to the search script is not perfect, we can't exit here (check manually)
echo "The package shouldn't include binary file, but get $COUNT"
Copy link
Member

@imbajin imbajin Feb 13, 2023

Choose a reason for hiding this comment

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

Find a critical problem here, we need exclude the files with a white-list & exit here:

  1. *.txt should be allowed, but why grep -v *.txt doesn't work?
  2. exclude the file /hugegraph-hubble/hubble-fe/src/assets/imgs/logo.png
  3. exclude the file ./hugegraph-hubble/hubble-fe/public/favicon.ico
  4. exclude the file yarn.lock

This is the 1st priority we need to fix and enhance it

Copy link
Member Author

@z7658329 z7658329 Feb 13, 2023

Choose a reason for hiding this comment

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

Find a critical problem here, we need exclude the files with a white-list & exit here:

  1. *.txt should be allowed, but why grep -v *.txt doesn't work?
  2. exclude the file /hugegraph-hubble/hubble-fe/src/assets/imgs/logo.png
  3. exclude the file ./hugegraph-hubble/hubble-fe/public/favicon.ico
  4. exclude the file yarn.lock

This is the 1st priority we need to fix and enhance it

1.should exit while check failed ?
2.use 'grep --exclude=*.{txt}' to exclude *.txt , and similar to others ?
exclude works in my repo test,see: https://github.com/z7658329/incubator-hugegraph-doc/actions/runs/4168898449/jobs/7216220550

@@ -212,8 +236,6 @@ jobs:
cd dist/${{ inputs.release_version }}
cd ./*hugegraph-incubating*${{ inputs.release_version }} || exit
bin/init-store.sh && sleep 1
# kill the HugeGraphServer process by jps
jps | grep HugeGraphServer | awk '{print $1}' | xargs kill -9
Copy link
Contributor

Choose a reason for hiding this comment

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

we must stop the old one, otherwise can't start the new one

@@ -245,8 +267,12 @@ jobs:
cat conf/hugegraph-hubble.properties
bin/stop-hubble.sh && bin/start-hubble.sh
cd - || exit
# TODO: need stop the server here
jps | grep HugeGraphServer | awk '{print $1}' | xargs kill -9
Copy link
Contributor

Choose a reason for hiding this comment

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

sleep a while before the stop, like 60s?

@@ -180,6 +182,7 @@ jobs:
echo "test hubble"
cd ./*hubble*${{ inputs.release_version }} || exit
cat conf/hugegraph-hubble.properties && bin/start-hubble.sh
# TODO: need stop the server here
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's difficult to find a time to stop, we can close the old one before starting server.
image

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

@github-actions
Copy link

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label

@imbajin
Copy link
Member

imbajin commented Apr 7, 2023

@z7658329 merge this PR first, and enhance it later (due to we lack the script now 😢)

Address the comment in another PR

@imbajin imbajin merged commit 1167b17 into apache:master Apr 7, 2023
github-actions bot pushed a commit that referenced this pull request Apr 7, 2023
* add validate-release.sh

---------

Co-authored-by: imbajin <jin@apache.org> 1167b17
github-actions bot pushed a commit to zoroqi/incubator-hugegraph-doc that referenced this pull request Apr 7, 2023
* add validate-release.sh

---------

Co-authored-by: imbajin <jin@apache.org> 1167b17
@javeme
Copy link
Contributor

javeme commented Feb 25, 2024

Address the comment in another PR

@imbajin could you please check all the comments are addressed?
https://github.com/apache/incubator-hugegraph-doc/blob/master/dist/validate-release.sh

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.

[Bug] validate-release.sh improvement on Mac
3 participants