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

Joshua s brown automate gcs setup #946

Merged

Conversation

JoshuaSBrown
Copy link
Collaborator

@JoshuaSBrown JoshuaSBrown commented Apr 6, 2024

Description

This pr helps to streamline the creation of the dev environment using two docker compose instances the first for the core services:

  • datafed-core
  • datafed-web
  • arangodb

The second for the datafed repo services.

  • datafed-gcs
  • datafed-repo

Tasks

  • Upgrade version
  • Changelog comment
  • Add tests
  • Feature description
  • Working tests in ci
  • Formatting passes
  • Documentation added
  • Add labels
  • Assign work
  • Assign expected time

JoshuaSBrown and others added 30 commits February 19, 2024 10:38
@JoshuaSBrown JoshuaSBrown self-assigned this Apr 9, 2024
Copy link
Collaborator

@nedvedba nedvedba left a comment

Choose a reason for hiding this comment

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

Overall, I only noticed a few small spelling mistakes, unused code, and potentially unnecessary debug information, as most the PR was just cleaning up files, using environment variables better instead of hard coding paths, and the actual code committed was simple and readable.

CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines +20 to +27
if(BUILD_SHARED_LIBS)
add_library( common SHARED ${Sources})
target_link_libraries( common PUBLIC ${DATAFED_BOOST_DATE_TIME_LIBRARY_PATH} protobuf::libprotobuf libzmq datafed-protobuf)
else()
add_library( common STATIC ${Sources})
target_link_libraries( common PUBLIC ${DATAFED_BOOST_DATE_TIME_LIBRARY_PATH} protobuf::libprotobuf libzmq-static datafed-protobuf)
endif()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised this is so simple to be able to swap between shared and static libraries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it's not, for instance protobuf and some of the other libraries when built will output a cmake file that defaults to shared. Really, it's in building the dependencies and the fact that the CMake files they create are often poorly written. Many times building the dependency if it includes both a shared and static option CMake will only link with the shared library if it is found anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really if you look at the find_package command that's where all the problems are.

Comment on lines -117 to +118
if (m_log_level >= LogLevel::TRACE) {
if (static_cast<unsigned int>(m_log_level) >=
static_cast<unsigned int>(LogLevel::TRACE)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is to not rely on C++ loose typing which is probably a good idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could probably leave this out, because I explicitly make the enum an unsigned int when it is constructed. Though I'm as you might have noticed a fan of being explicit sometimes at the detriment of clarity. If you think it makes sense to remove it we can.

compose/README.md Outdated Show resolved Hide resolved
compose/README.md Outdated Show resolved Hide resolved
Comment on lines 327 to 336
#cmake -S . -B build \
# -DBUILD_SHARED_LIBS=ON \
# -DCMAKE_POSITION_INDEPENDENT_CODE=ON \
# -DCMAKE_INSTALL_PREFIX="${DATAFED_DEPENDENCIES_INSTALL_PATH}"
#cmake --build build -j 8
#if [ -w "${DATAFED_DEPENDENCIES_INSTALL_PATH}" ]; then
# cmake --build build --target install
#else
# "$SUDO_CMD" cmake --build build --target install
#fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments

repository/docker/entrypoint_repo.sh Outdated Show resolved Hide resolved
@@ -54,14 +78,22 @@ cat << EOF > mapping.json
EOF

#DOMAINS="--domain ornl.gov --domain clients.auth.globus.org --domain gmail.com"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

scripts/globus/setup_globus.sh Show resolved Hide resolved
@JoshuaSBrown
Copy link
Collaborator Author

@par-hermes format

@JoshuaSBrown
Copy link
Collaborator Author

@par-hermes format

@JoshuaSBrown
Copy link
Collaborator Author

@par-hermes format

@JoshuaSBrown JoshuaSBrown merged commit a858afd into JoshuaSBrown-upgrade-protobuf Apr 10, 2024
5 of 6 checks passed
@JoshuaSBrown JoshuaSBrown deleted the JoshuaSBrown-automate-gcs-setup branch April 10, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Build Related to the build system Component: Core Relates to core service Component: Database Relates to database microservice / data model Component: Repository Relates to repository service Component: Scripts Helper scripts or admin scripts Component: Web UI Relates to web appp user interface Priority: High Highest priority Type: New Feature New or enhanced feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants