-
Notifications
You must be signed in to change notification settings - Fork 303
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
[FLINK-30768] [Project Website] flink-web version cleanup #683
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up, @victor9309 . I added two comments. I'm curious about your opinion on that. PTAL
Thanks @XComp for the review. modify _include/q/gradle-quickstart.sh and extend the list variable that's already present in docs/config.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments. I have a few things that we need to address still. PTAL
Thanks @XComp for the review. I changed the config directory, please check to see if it is correct |
Thanks @XComp for the review. Thank you very much for your advice.
I found that exit 1 in the function can not exit the script, so I modify the logic, please check it
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another thorough pass over it. PTAL
Just as a heads-up: There should be a conflict in this PR popping up soon because of the 1.18.0 release happening right now.
_include/q/_utils.sh
Outdated
SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) | ||
CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" | ||
|
||
export TOP_PID=$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export TOP_PID=$$ | |
# fatal error handling | |
export PROCESS_PID=$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed the renaming of the variable. I'm not sure what TOP stands for in this context. 🤔 Alternatively, you can replace "TOP" with the long version of this acronym.
Thanks @XComp for the review. I really feel what you're suggesting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you. :-) Just a minor discussion point on the variable name.
_include/q/_utils.sh
Outdated
SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) | ||
CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" | ||
|
||
export TOP_PID=$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed the renaming of the variable. I'm not sure what TOP stands for in this context. 🤔 Alternatively, you can replace "TOP" with the long version of this acronym.
_include/q/quickstart-scala.sh
Outdated
mvn archetype:generate \ | ||
-DarchetypeGroupId=org.apache.flink \ | ||
-DarchetypeArtifactId=flink-quickstart-scala \ | ||
-DarchetypeVersion=${1:-1.17.0} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why it didn't occur to me before but ./quickstart-scala.sh
is failing since 1.17. This is because we removed scala support in 1.17. I created FLINK-33383 to cover this issue.
Thanks @XComp for the review. I changed the variable; If _includes/q/quickstart-scala.sh and _includes/q/quickstart-scala-SNAPSHOT.sh are removed, and if 1.7+ is not supported; Do you think that's okay if I don't make any changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 I added a few separate commits (e.g. reverting the log4j/slft4j version changes because I noticed that we don't necessarily have to align this version with the versions used in Flink). I felt bad to let you do this change again. That is why I did that in the end. I pushed the changes for transparency reasons.
I will squash the commits and rebuild the website. Thanks for your help 👍
…rely into docs/config.toml
What is the purpose of the change
update versions (through variables) and adding references that these variables have to be updated to the corresponding versions that used in the Flink source code as well.
Brief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation