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

TRUNK-6214: Update OpenMRS version in fix_liquibase_snapshots.sh scri… #4536

Merged
merged 13 commits into from
Apr 5, 2024

Conversation

Seremba
Copy link
Contributor

@Seremba Seremba commented Jan 18, 2024

…pt to current release

Description of what I changed

Issue I worked on

see https://openmrs.atlassian.net/browse/TRUNK-6214

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@@ -17,6 +17,6 @@
# The updated snapshots are written to the openmrs-core/liquibase/snapshots folder.
#

openmrs_version=2.4.0-SNAPSHOT
openmrs_version=2.6.0-SNAPSHOT
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 extract the current OpenMRS version directly from the pom.xml file, So we no need to manually update the OpenMRS version during the release of a new version.

@@ -17,6 +17,9 @@
# The updated snapshots are written to the openmrs-core/liquibase/snapshots folder.
#

openmrs_version=2.4.0-SNAPSHOT
pom_path="/mnt/c/Users/Admin/Desktop/development/openmrs-dev/openmrs-core/liquibase/pom.xml"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to suggest using relative paths instead absolute path. Absolute paths can lead to issues when the project is moved to a different directory or when others are working on the project from different environments.

@Seremba
Copy link
Contributor Author

Seremba commented Jan 24, 2024

Hi @ManojLL I am done adding the relative path

Comment on lines 19 to 47

# Get the directory of the script
script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"

# Search for the openmrs-core directory
project_dir="$script_dir"
while [[ "$project_dir" != "/" ]]; do
if [[ -d "$project_dir/openmrs-core" ]]; then
break
fi
project_dir="$(dirname "$project_dir")"
done

if [[ ! -d "$project_dir/openmrs-core" ]]; then
echo "Error: 'openmrs-core' directory not found"
exit 1
fi

# Construct the path to pom.xml and Check if the provided path exists
pom_path="$project_dir/openmrs-core/liquibase/pom.xml"

if [ ! -f "$pom_path" ]; then
echo "Error: $pom_path not found."
exit 1
fi

version_line=$(grep '<version>' "$pom_path")

openmrs_version=$(echo "$version_line" | awk -F'[<>]' '{print $3}')
Copy link
Contributor

Choose a reason for hiding this comment

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

according to the instructions , we have to execute this script from openmrs-core/liquibase. Then we do not need to consider about the root folder things.

Instead, we can use this openmrs_version=$(grep -m 1 '<version>' pom.xml | sed -n 's/.*<version>\(.*\)<\/version>.*/\1/p') .

@Seremba
Copy link
Contributor Author

Seremba commented Feb 7, 2024

@ManojLL @dkayiwa kindly review this.

@Seremba
Copy link
Contributor Author

Seremba commented Feb 22, 2024

Hi @ibacher @ManojLL @dkayiwa I finished this PR over three weeks ago, but I request its review. Would you like me to repeat it? Is there anything to correct? Thanks a lot.

@dkayiwa
Copy link
Member

dkayiwa commented Feb 22, 2024

Can you include a link to the ticket as advised here? https://wiki.openmrs.org/display/docs/Pull+Request+Tips

@Seremba
Copy link
Contributor Author

Seremba commented Feb 22, 2024

Thanks @dkayiwa , I am done.

@dkayiwa
Copy link
Member

dkayiwa commented Feb 22, 2024

@Seremba did you test this and confirm that openmrs_version is assigned the appropriate value?

@Seremba
Copy link
Contributor Author

Seremba commented Feb 22, 2024

Yes @dkayiwa, I surely did!

@dkayiwa
Copy link
Member

dkayiwa commented Feb 22, 2024

How did you test it?

@Seremba
Copy link
Contributor Author

Seremba commented Feb 22, 2024

I cd into openmrs\liquibase\srcipts and then ran bash fix_liquibase_snapshots.sh then it printed this as shown in the picture.
Screenshot 2024-02-22 183934
interest was in the fact that it displays the openmrs_version. That is how I tasted it @dkayiwa

@Seremba
Copy link
Contributor Author

Seremba commented Feb 22, 2024

I cd into openmrs\liquibase\srcipts and then ran bash fix_liquibase_snapshots.sh then it printed this as shown in the picture. Screenshot 2024-02-22 183934 interest was in the fact that it displays the openmrs_version. That is how I tasted it @dkayiwa

The screenshot below is for the code.
Screenshot 2024-02-22 184100


java -jar ./target/openmrs-liquibase-${openmrs_version}-jar-with-dependencies.jar
openmrs_version=$(grep -m 1 '<version>' ../pom.xml | sed -n 's/.*<version>\(.*\)<\/version>.*/\1/p' | awk '{$1=$1;print}')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
openmrs_version=$(grep -m 1 '<version>' ../pom.xml | sed -n 's/.*<version>\(.*\)<\/version>.*/\1/p' | awk '{$1=$1;print}')
openmrs_version=$(mvn help:evaluate -Dexpression=project.version -q -DforceStdout)

@Seremba
Copy link
Contributor Author

Seremba commented Mar 1, 2024

Hello @dkayiwa @ManojLL and @ibacher, kindly review this.
Screenshot 2024-03-01 090310

Screenshot 2024-03-01 090459

the two screenshots show that code extracts the openmrs_version from pom.xml using maven as I suggested by @ibacher. Thanks a lot!

@Seremba
Copy link
Contributor Author

Seremba commented Mar 30, 2024

@dkayiwa @ibacher @ManojLL is there anything else I am meant to do on this PR?

openmrs_version=$(cd ../ && mvn help:evaluate -Dexpression=project.version -q -DforceStdout)


java -jar ./target/openmrs-liquibase-"${openmrs_version}"-jar-with-dependencies.jar
Copy link
Member

Choose a reason for hiding this comment

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

What was the motivation behind putting ${openmrs_version} in double quotes?

@Seremba
Copy link
Contributor Author

Seremba commented Apr 5, 2024

I have removed the double quotes @dkayiwa

@dkayiwa dkayiwa merged commit d4ec1d7 into openmrs:master Apr 5, 2024
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants