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

Add minimal MariaDB container for WMAgent #1412

Merged
merged 17 commits into from
Mar 5, 2024

Conversation

todor-ivanov
Copy link
Contributor

@todor-ivanov todor-ivanov commented Sep 1, 2023

fixing dmwm/WMCore#11313

MariaDB default image for running WMAgent

Prerequisites

This PR is the initial setup of the MariaDB container for WMAgent.
This image inherits from the mainstream mariadb one, and follows the same tagging schema. On top of the base mariadb image we add all the structure needed for running the WMAgent with MariaDB and two main scripts:

  • mariadb-docker-run.sh
  • mariadb-docker-build.sh

For building the containers, and for creating the mount area at the host and the the bind mounts inside the container, respectively. Those are as follows:

At the host:

/data/dockerMount/{admin|srv}/mariadb

At the container:

/data/{admin|srv}/mariadb

Upon starting the container we try to initialize the default user and system databases, which if previously created should exist in the host mount area. And the last steps are creating the wmagent database.

There are no other external dependencies.
We fetch all the passwords from two secrets files:

  • /data/admin/wmagent/WMAgent.secrets - for reading the credentials for the user to be used by the WMAgent to connect to the datbase
  • /data/admin/mariadb/MariaDB.secrets - for reading the the credentials for the root user who is about to have full administrative rights on the MariaDB server
    NOTE: The server admin user configured at the MariaDB.secrets file, must match the username of the one who is to run the server inside the container. And the later is resolved at runtime, depending on where we run the container, it could be on of the three:
    • CERN - production agent
    • CERN - T0 agent
    • FNAL

FYI: @amaltaro

@khurtado
Copy link
Contributor

Hi @todor-ivanov
It seems the default MariaDB 11.1 used in this PR does not include mysql commands anymore, as opposed to e.g.: 10.x

What version did you use to test?

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Feb 23, 2024

Hi @khurtado Yes indeed they have removed all myslq related symlinks to the commands in v11.0:

For local tests directly on the host I used some really old version which was closer to the current rpm based version we use - 5.5.68, but there is no official mariadb docker container uploaded in the public repositories with a version that old. You can try mariadb container 10.11instead. I just tested it and it is working properly to some later stage when it tries to create the users because we will have to parse the previously mounted WMASecrets file properly. And due to some messed up paths between host and container mount points . I am working now to fix that as well.
And I think, this is the proper moment for us as well, to move away from myslq* named executables to the modern world.

@amaltaro
Copy link
Contributor

@todor-ivanov Todor, I am not sure where you came from with the version 5.5.68, but we have been using version 10.6.5 for 3 years now: https://github.com/cms-sw/cmsdist/blob/comp_gcc630/mariadb.spec#L1

@todor-ivanov
Copy link
Contributor Author

hi @amaltaro I do not remember, but it doesn't matter already ... I have already fixed to the latest version that still has the mysql naming soft links, which is 10.11. and I am testing the container now. We can later move to 11.0 once we get through the proper startup sequence.

@todor-ivanov
Copy link
Contributor Author

hi @amaltaro,
Actually, since you asked, I've scratched my head a little and I remember now. As I mentioned previously for my local tests I used the system package for MariaDB coming from the OS. Current our WMAgents are still cc7 so I was bound to this very old version:

# yum info mariadb 
Loaded plugins: changelog, kernel-module, ovl, post-transaction-actions, priorities, tsflags, versionlock
2671 packages excluded due to repository priority protections
Installed Packages
Name        : mariadb
Arch        : x86_64
Epoch       : 1
Version     : 5.5.68
Release     : 1.el7
Size        : 49 M
Repo        : installed
From repo   : base
Summary     : A community developed branch of MySQL
URL         : http://mariadb.org
License     : GPLv2 with exceptions and LGPLv2 and BSD
Description : MariaDB is a community developed branch of MySQL.
            : MariaDB is a multi-user, multi-threaded SQL database server.
            : It is a client/server implementation consisting of a server daemon (mysqld)
            : and many different client programs and libraries. The base package
            : contains the standard MariaDB/MySQL client programs and generic MySQL files.

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Feb 23, 2024

And with my latest commit all is fixed and we are aligned with the rest of the world to the latest MariDB version: 11.0

@khurtado you may give it a try. But before we merge this PR we have to:

  • Fetch the MariaDB password from WMAgentSecrets file (which is supposed to be previously mounted from the host)
  • link start_mysql from run.sh

I'll work on that later tonight as well.

@amaltaro
Copy link
Contributor

@todor-ivanov Todor, did you check the release notes to see any significant changes between MariaDB 10.x and 11.x?

As much as I like moving forward with these versions and upgrades, at this stage I feel like it would be important to make the minimum amount of changes compared to how we run the agent at this moment.

We haven't done scalability and performance tests yet, and it would help to be able to compare apples to apples.

@khurtado
Copy link
Contributor

khurtado commented Feb 23, 2024

@todor-ivanov Thank you! I made some minor changes to the start-mariadb.sh to get the user/pass from the secrets and tried starting with some errors.

2024-02-23 20:37:22 0 [Note] InnoDB: Loading buffer pool(s) from /data/srv/mariadb/11.0/install/database/ib_buffer_pool
2024-02-23 20:37:22 0 [ERROR] Could not open mysql.plugin table: "Table 'mysql.plugin' doesn't exist". Some plugins may be not loaded
2024-02-23 20:37:22 0 [Warning] 'innodb-file-format' was removed. It does nothing now and exists only for compatibility with old my.cnf files.
2024-02-23 20:37:22 0 [Warning] 'innodb-large-prefix' was removed. It does nothing now and exists only for compatibility with old my.cnf files.
2024-02-23 20:37:22 1 [Warning] Failed to load slave replication state from table mysql.gtid_slave_pos: 1017: Can't find file: './mysql/' (errno: 2 "No such file or directory")

But what I wanted to note more than the errors are these 2 lines:

2024-02-23 20:37:22 0 [Warning] 'innodb-file-format' was removed. It does nothing now and exists only for compatibility with old my.cnf files.
2024-02-23 20:37:22 0 [Warning] 'innodb-large-prefix' was removed. It does nothing now and exists only for compatibility with old my.cnf files.

So, it looks like we have the following in the configuration:

# default: Barracuda
innodb_file_format=Barracuda
# default: ON
innodb_file_per_table=ON
# supports prefix index larger than 767 bytes (might be already implicit in the DYNAMIC mode?)
# default: ON
innodb_large_prefix=ON

But these settings are not supported in MariaDB 11.x anymore. I don't know how much we depend on these settings and if they could lead to problems with the current system (e.g.: does 11.x support large prefixes by default without any settings now?). @amaltaro may know more but I guess trying 10.6.5 which is the version currently used in production would be a good first step to avoid unexpected errors.

@amaltaro
Copy link
Contributor

Thanks for this information, Kenyi. This reinforces even more my request to not make any changes to the stack at this stage, unless there is no other way.

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Feb 24, 2024

Hi @khurtado @amaltaro
I was thinking about improving the startup sequence and the need of proper parsing of the Secrets files. It resulted in my latest commit. Basically I reused some of the approaches from the WMAgent container for parsing the secrets file and included everything in the manage script. So no need for a separate start-mariadb.sh file, and the whole set of environment variables involved in the process is now configured only through the Docker file, which is the proper way.
I also called the manage script from the default entry point for the container run.sh, so now the MariaBD server is properly initialized and started with the container startup. And the startup log goes in /data/run.log.

There was one more thing I had to do though, in order to properly grant database permissions to users.

  • I had to setup a separate database root user (I used the cmst1 user for that) with full server administrative power. This username and passowrd are read from /data/admin/mariadb/MariaDB.secrets - from inside the container and /data/dockerMount/admin/mariadb/MariaDB.secrets - from the host respectively.
  • I had to give privileges to the WMAgent database user only on the wmagent database. This username and password are read from /data/admin/wmagent/WMAgent.secrets - from inside the container and /data/dockerMount/admin/wmagent/WMAgent.secrets - from the host respectively.

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Feb 24, 2024

@amaltaro about the version change it is now just a matter of one parameter passed at build time and it really costs nothing to move between mariadb versions. Here is how it goes:

cmst1@vocms0290:wmagent-mariadb $ ./mariadb-docker-build.sh -t 11.0
...
cmst1@vocms0290:wmagent-mariadb $ ./mariadb-docker-build.sh -t 10.11
...
cmst1@vocms0290:wmagent-mariadb $ docker image ls 
REPOSITORY      TAG       IMAGE ID       CREATED          SIZE
local/mariadb   10.11     07b81ede2660   3 minutes ago    948MB
local/mariadb   latest    07b81ede2660   3 minutes ago    948MB
local/mariadb   11.0      fb4deccd8e73   32 minutes ago   948MB

and to start the version we want is just

cmst1@vocms0290:wmagent-mariadb $ docker kill mariadb
mariadb

cmst1@vocms0290:wmagent-mariadb $ ./mariadb-docker-run.sh -t 10.11
Starting the mariadb:10.11 docker container with the following parameters:  --user cmst1
b07eb64ad0d4662527b15e8dee9565f297e3c6edbad69e07bce8e0e3d3b3476c

cmst1@vocms0290:wmagent-mariadb $ docker exec -it mariadb bash

(MariaDB-10.11) [cmst1@vocms0290:data]$ 
(MariaDB-10.11) [cmst1@vocms0290:data]$ manage status 

mariadb-admin  Ver 10.0 Distrib 10.11.7-MariaDB, for debian-linux-gnu on x86_64
Copyright (c) 2000, 2018, Oracle, MariaDB Corporation Ab and others.

Server version		10.11.7-MariaDB-1:10.11.7+maria~ubu2204-log
Protocol version	10
Connection		Localhost via UNIX socket
UNIX socket		/data/srv/mariadb/10.11/mariadb.sock
Uptime:			34 sec

Threads: 1  Questions: 23  Slow queries: 0  Opens: 16  Open tables: 10  Queries per second avg: 0.676

Uptime: 34  Threads: 1  Questions: 24  Slow queries: 0  Opens: 16  Open tables: 10  Queries per second avg: 0.705

I do not think we must move/change the MariaDB version right now. We can reuse whatever Mariadb version we currently use for our agents and upgrade whenever we want to upgrade. It will cost us nothing in terms of operations.

@khurtado I have not changed the my.cnf configuration at the moment. But if we observe the same errors in the with the version we currently use for the agents, then we may consider removing those parameters.

Copy link
Collaborator

@vkuznet vkuznet left a comment

Choose a reason for hiding this comment

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

Overall the content is fine but you can reduce number of build steps at least to 5 less steps by re-arranging lines.

docker/pypi/wmagent-mariadb/Dockerfile Outdated Show resolved Hide resolved
@@ -0,0 +1,81 @@
ARG TAG=11.0
ARG MDB_TAG=$TAG
FROM mariadb:${MDB_TAG}
Copy link
Collaborator

Choose a reason for hiding this comment

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

usually we start all Dockerfiles with FROM and MAINTAINER, then add additional stuff. I suggest to keep it consistent and move ARG below these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually need to start with the MDB_ARG, because we need it for referring to the base mariadb image to start from. It comes as an build option. But I've got rid of the additional TAG argument, though.

docker/pypi/wmagent-mariadb/Dockerfile Show resolved Hide resolved
docker/pypi/wmagent-mariadb/Dockerfile Show resolved Hide resolved
docker/pypi/wmagent-mariadb/Dockerfile Outdated Show resolved Hide resolved
@khurtado
Copy link
Contributor

@todor-ivanov Just for the record, I tested as following:

First, secret files

# Created /data/dockerMount/admin/wmagent/WMAgent.secrets
MDB_USER=cmst1

MDB_PASS=mypass

# Created /data/dockerMount/admin/wmagent/MariaDB.secrets
MDB_ROOT=root
MDB_ROOTPASS=myRootpass

Then, built via

 ./mariadb-docker-build.sh -t 10.6.5

And then run:

./mariadb-docker-run.sh -t 10.6.5

But I ran into permission issues. In order to make it work I had to uncomment the following last line below and remove the group (since my test agent for example, does not have a cmst1 group.

[[ -d $HOST_MOUNT_DIR/srv/mariadb/$MDB_TAG/logs ]] || { mkdir -p $HOST_MOUNT_DIR/srv/mariadb/$MDB_TAG/logs ;} || exit $?

sudo chown -R $mariadbUser $HOST_MOUNT_DIR/srv/mariadb/$MDB_TAG

Before making or requesting any changes for the review, is this a bug, or did I miss something while deploying?

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Feb 26, 2024

Thanks for testing this @khurtado I have deliberately commented this line, but kept it in the script, because it is supposed for us to have the cmst1 user and group in all our agents. But it is not mandatory for the user to have sudo permissions. So this should not be a bug, but may require to run the appropriate command for changing the main directory ownership once, like that:

sudo chown -R cmst1:zh /data/dockerMount/srv

@todor-ivanov
Copy link
Contributor Author

@vkuznet I think I have addressed all you comments. Please take a look again.

docker/pypi/wmagent-mariadb/Dockerfile Show resolved Hide resolved
docker/pypi/wmagent-mariadb/Dockerfile Outdated Show resolved Hide resolved
docker/pypi/wmagent-mariadb/Dockerfile Outdated Show resolved Hide resolved
docker/pypi/wmagent-mariadb/Dockerfile Show resolved Hide resolved
docker/pypi/wmagent-mariadb/Dockerfile Show resolved Hide resolved
docker build $dockerOpts -t local/mariadb:$MDB_TAG -t local/mariadb:latest .

$PUSH && {
docker login registry.cern.ch
Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt it should be here, it is safer to leave it out from the script and execute outside of it.

Copy link
Contributor Author

@todor-ivanov todor-ivanov Feb 27, 2024

Choose a reason for hiding this comment

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

Hi @vkuznet I finally made it work properly. I added this option in purpose. Because we are about to get into a mess with local and remote images if we do not maintain a hard tagging schema. So whenever one adds the -p option to either mariadb-docker-run.sh or mariadb-docker-build.sh it is always going to be an image pulled/pushed from/to registry.cern.ch to be run or uploaded respectively. let me put it in an example to visualize what I mean:

Imagine one is working on building a new tag and he/she has uploaded already a version in the registry, but still continued working on the local build and ends up with two versions of the same tag one uploaded and one local. If he was about to use the wrapper mariadb-docker-build.sh he/she would have ended with the following set of images at the host:

cmst1@vocms0290:wmagent-mariadb $ docker image  ls 
REPOSITORY                 TAG       IMAGE ID       CREATED          SIZE
local/mariadb              10.6.5    4efa646aea3e   6 minutes ago    950MB
local/mariadb              latest    4efa646aea3e   6 minutes ago    950MB
registry.cern.ch/mariadb   10.6.5    8539e03b7a1d   21 minutes ago   950MB
registry.cern.ch/mariadb   latest    8539e03b7a1d   21 minutes ago   950MB

Looking at the repository names it becomes immediately clear which image comes from where. And looking at the image hashes you can clearly see that the version uploaded at the registry differs from the version at the local repo.

Now trying to run those again with the mariadb-docker-run.sh wrapper, immediately tells you which is the image you are currently running. And you point to the one you want again just by adding/removing the -p parameter to the command:

Running the local one:

cmst1@vocms0290:wmagent-mariadb $ ./mariadb-docker-run.sh -t 10.6.5
Starting the mariadb:10.6.5 docker container with the following parameters:  --user cmst1
eb7e0d879d4d7fa597587c734837c5289886a6aaf6a82c072187371fdf312b90

cmst1@vocms0290:wmagent-mariadb $ docker ps 
CONTAINER ID   IMAGE                  COMMAND      CREATED         STATUS         PORTS     NAMES
eb7e0d879d4d   local/mariadb:10.6.5   "./run.sh"   3 seconds ago   Up 2 seconds             mariadb

Running the one from the remote registry:

cmst1@vocms0290:wmagent-mariadb $ ./mariadb-docker-run.sh -t 10.6.5 -p
Pulling Docker image: registry.cern.ch/cmsweb/mariadb:10.6.5
10.6.5: Pulling from cmsweb/mariadb
Digest: sha256:61f798b55a1c743686e1568509975308dc07b5b24486894053d6a312983c4af6
Status: Downloaded newer image for registry.cern.ch/cmsweb/mariadb:10.6.5
registry.cern.ch/cmsweb/mariadb:10.6.5
Starting the mariadb:10.6.5 docker container with the following parameters:  --user cmst1
21d9c6598f35e627834d1b796460047605d6255cebc746d572289c7b418053ed

cmst1@vocms0290:wmagent-mariadb $ docker ps 
CONTAINER ID   IMAGE                             COMMAND      CREATED         STATUS         PORTS     NAMES
21d9c6598f35   registry.cern.ch/mariadb:10.6.5   "./run.sh"   7 seconds ago   Up 6 seconds             mariadb

This is an approach I have adopted and gradually polished with all three types of containers we are about to run for the WMAgent.

And, Yes, I agree with you this may create a mess with the credentials if many people use the same user for uploading. That's why with my latest commit:

  • I Added an explicit check for the login name of the user to build/connect to the ccern registry and if it is not the current one (i.e. someone have switched to cmst1 or other user before uploading) the script just aborts and does not proceed with creating a login session.
  • I explicitly destroy the login session at the end of the execution - this will remove the credentials preserved in the user's home.

It is another story if we are willing to configure credential helpers for these operations. I'd say we should, but they do not work properly on our cc7 machines(i.e. the pass command is missing in our agents). So as long as we are about to be working with WMAgent images on our current agents, we are bound to temporarily storing credentials in the config file for the time the lgin session is active. But this should be only during this transient period. Once we move to alma9 agents this would be easily fixable as well.

docker/pypi/wmagent-mariadb/run.sh Show resolved Hide resolved
@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Feb 27, 2024

ok I have it all implemented now following my second suggestion - (by adding all the 3 users in the container and resolving the username to run the container at runtime), but I have not yet pushed the commit to this PR here, because this approach would require to add the following information to the Docker file:

useradd -u <CERN WMAgent UID>  -g <CERN WMAgent GID>  -G 999 -m <CERN WMAgent username> 
useradd -u <T0 Agent UID>  -g <T0 Agent GID>  -G 999 -m <T0 Agent username> 
useradd -u <FNAL WMAgent UID>  -g <FNAL WMAgentGID>  -G 999 -m  <FNAL WMAgent username> 

@amaltaro @khurtado @vkuznet If we all do not consider this as a security risk to add these numbers here, I can proceed and push the commit. And with this the current PR will be completed.

Let me know what you think?

docker/pypi/wmagent-mariadb/Dockerfile Outdated Show resolved Hide resolved
docker/pypi/wmagent-mariadb/run.sh Show resolved Hide resolved
docker/pypi/wmagent-mariadb/mariadb-docker-build.sh Outdated Show resolved Hide resolved
docker/pypi/wmagent-mariadb/mariadb-docker-build.sh Outdated Show resolved Hide resolved
@todor-ivanov
Copy link
Contributor Author

@amaltaro @khurtado @vkuznet If we all do not consider this as a security risk to add these numbers here, I can proceed and push the commit. And with this the current PR will be completed

Since the information for the cmst1 user is already in the WMAgent docker file I am going to proceed with this commit adding only this user for the time being in order to be able to progress and later we can add the other two users who are supposed to run this setup as well.

@todor-ivanov
Copy link
Contributor Author

@khurtado the issue with the user running the database is resolved now. Could you give it another try.

p.s. Please run the container with user cmst1, and for your initial run, also consider removing the old directory with the broken permissions:

sudo rm -rf   /data/dockerMount/srv/mariadb/

@todor-ivanov
Copy link
Contributor Author

thanks @vkuznet

@khurtado let me know the results of your final tests and if they are positive we can merge it.
I also uploaded the tag 10.6.5 to registry.cern.ch , so you can test it from both: local and upstream repository as well.

@khurtado
Copy link
Contributor

khurtado commented Feb 29, 2024

ok I have it all implemented now following my second suggestion - (by adding all the 3 users in the container and resolving the username to run the container at runtime), but I have not yet pushed the commit to this PR here, because this approach would require to add the following information to the Docker file:

useradd -u <CERN WMAgent UID>  -g <CERN WMAgent GID>  -G 999 -m <CERN WMAgent username> 
useradd -u <T0 Agent UID>  -g <T0 Agent GID>  -G 999 -m <T0 Agent username> 
useradd -u <FNAL WMAgent UID>  -g <FNAL WMAgentGID>  -G 999 -m  <FNAL WMAgent username> 

@amaltaro @khurtado @vkuznet If we all do not consider this as a security risk to add these numbers here, I can proceed and push the commit. And with this the current PR will be completed.

Let me know what you think?

I don't think exposing the uid/gids imply a security risk, since we are not providing any information on how to log in to those accounts. We would just need to be sure that these uids/gids are consistent across all FNAL, T0 agent and CERN nodes. I think that is indeed the case for CERN, but I don't know about the T0/FNAL cases. If not, an alternative is to use the usernames which we do know 100% are consistent across all nodes and dynamically read the uid/gid information in the build.sh script before invoking the docker build.

EDIT: I can confirm uid/gid are the same across production and testbed FNAL agents, so it will likely be the same for t0

Copy link
Contributor

@khurtado khurtado left a comment

Choose a reason for hiding this comment

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

Looking good with me. Just a few minor comments on the usage example and paramenters description.

docker/pypi/wmagent-mariadb/mariadb-docker-run.sh Outdated Show resolved Hide resolved
docker/pypi/wmagent-mariadb/mariadb-docker-run.sh Outdated Show resolved Hide resolved
@khurtado
Copy link
Contributor

@todor-ivanov I just tested on my CERN agent and the server is running fine. Latest test was using the image from the registry you uploaded.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

@todor-ivanov Todor, these changes are looking pretty good. I left a few comments that are worth attending to.

In addition, I would like to ask you to:

  • update the pull request description with some short and useful information. Any dependencies (packages, directories, new secrets, etc)? What is the default running options (local or pull)? Etc
  • the manage script has inconsistent usage of the echo command. Given that we are getting started with all this development, it would be great to pick one and stick with that.
  • is there any changes to the default service configuration my.cnf compared with what we currently have in the deployment repo? If so, please clarify what are those.

docker/pypi/wmagent-mariadb/manage Show resolved Hide resolved
docker/pypi/wmagent-mariadb/manage Outdated Show resolved Hide resolved
docker/pypi/wmagent-mariadb/manage Outdated Show resolved Hide resolved
@todor-ivanov
Copy link
Contributor Author

Hi @amaltaro,

  • update the pull request description with some short and useful information. Any dependencies (packages, directories, new secrets, etc)? What is the default running options (local or pull)? Etc

Done

  • the manage script has inconsistent usage of the echo command. Given that we are getting started with all this development, it would be great to pick one and stick with that.

Done

  • is there any changes to the default service configuration my.cnf compared with what we currently have in the deployment repo? If so, please clarify what are those.

There were indeed, two minor changes I had to previously do at my.cnf to adapt for the limited resources I was using for testing. I've fixed those now. The only one that is currently different is that we now bind the server to IP: 127.0.0.1

Please take a look at the final result

@todor-ivanov
Copy link
Contributor Author

todor-ivanov commented Mar 1, 2024

BTW I also provided a README file including all the information one may need to operate with these containers.

https://github.com/dmwm/CMSKubernetes/blob/731e1bb5d85a58bffee195e3bb4577d3da754d9d/docker/pypi/wmagent-mariadb/README.md

FYI: @amaltaro @vkuznet @khurtado

@arooshap
Copy link
Member

arooshap commented Mar 1, 2024 via email

Copy link
Contributor

@khurtado khurtado left a comment

Choose a reason for hiding this comment

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

Looking good, thanks Todor!

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

These changes look good to me.
@khurtado please have a final look, and if you approve, we can get this merged.

@amaltaro
Copy link
Contributor

amaltaro commented Mar 1, 2024

@arooshap Aroosha, can you please merge this? I don't know if you you'd like to squash the commits or not, that's why I didn't merge it myself.

@khurtado
Copy link
Contributor

khurtado commented Mar 1, 2024

@todor-ivanov @amaltaro I just have one comment in the README.md, after this is addressed, I approve the merge as well :)

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.

5 participants