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

github api retries #108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions .github/workflows/build_nix_python.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
name: ubuntu-python-latest

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add security configurations to the workflow.

Consider adding the following security-related configurations:

  1. Explicit permissions to follow the principle of least privilege
  2. Timeout limits to prevent hanging jobs

Add these configurations after the concurrency block:

  cancel-in-progress: true

+ permissions:
+   contents: read
+
+ timeout-minutes: 30

Committable suggestion was skipped due to low confidence.

on:
pull_request:
branches: [ master ]

jobs:
build:
name: Python ${{ matrix.python-version }}
runs-on: ubuntu-latest

strategy:
fail-fast: true
max-parallel: 4
matrix:
python-version: ['3.11', '3.12']

steps:
- uses: actions/checkout@v4

- uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
architecture: x64

- run: |
./res/ci/nixish_setup.sh && make nix
sudo cp mkn /usr/bin
python3 -m pip install -r test/github/requirements.txt
mkn clean build -p github -dtOa "-fPIC -std=c++17"
./test/github/test_github_api.sh
Comment on lines +30 to +35
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance CI workflow with best practices.

Consider the following improvements to make the workflow more robust and efficient:

  1. Add step names for better visibility
  2. Implement dependency caching
  3. Split commands into separate steps
  4. Preserve build artifacts

Here's the suggested implementation:

-    - run: |
-        ./res/ci/nixish_setup.sh && make nix
-        sudo cp mkn /usr/bin
-        python3 -m pip install -r test/github/requirements.txt
-        mkn clean build -p github -dtOa "-fPIC -std=c++17"
-        ./test/github/test_github_api.sh
+    - name: Cache pip dependencies
+      uses: actions/cache@v3
+      with:
+        path: ~/.cache/pip
+        key: ${{ runner.os }}-pip-${{ hashFiles('test/github/requirements.txt') }}
+        restore-keys: |
+          ${{ runner.os }}-pip-
+
+    - name: Setup build environment
+      run: |
+        ./res/ci/nixish_setup.sh && make nix
+        sudo cp mkn /usr/bin
+
+    - name: Install Python dependencies
+      run: python3 -m pip install -r test/github/requirements.txt
+
+    - name: Build project
+      run: mkn clean build -p github -dtOa "-fPIC -std=c++17"
+
+    - name: Run tests
+      run: ./test/github/test_github_api.sh
+
+    - name: Upload build artifacts
+      uses: actions/upload-artifact@v3
+      with:
+        name: build-artifacts
+        path: |
+          bin/
+          lib/
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- run: |
./res/ci/nixish_setup.sh && make nix
sudo cp mkn /usr/bin
python3 -m pip install -r test/github/requirements.txt
mkn clean build -p github -dtOa "-fPIC -std=c++17"
./test/github/test_github_api.sh
- name: Cache pip dependencies
uses: actions/cache@v3
with:
path: ~/.cache/pip
key: ${{ runner.os }}-pip-${{ hashFiles('test/github/requirements.txt') }}
restore-keys: |
${{ runner.os }}-pip-
- name: Setup build environment
run: |
./res/ci/nixish_setup.sh && make nix
sudo cp mkn /usr/bin
- name: Install Python dependencies
run: python3 -m pip install -r test/github/requirements.txt
- name: Build project
run: mkn clean build -p github -dtOa "-fPIC -std=c++17"
- name: Run tests
run: ./test/github/test_github_api.sh
- name: Upload build artifacts
uses: actions/upload-artifact@v3
with:
name: build-artifacts
path: |
bin/
lib/

164 changes: 164 additions & 0 deletions inc/maiken/github.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,36 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include "maiken.hpp"
#ifdef _MKN_WITH_MKN_RAM_
#include "mkn/kul/yaml.hpp"
#include "mkn/ram/http.hpp"
#include "mkn/ram/https.hpp"

#include <chrono>
#include <string>
#include <thread>

namespace maiken {

namespace github {

static inline std::string URL = "api.github.com";
static inline int port = 443;
Comment on lines +48 to +49
Copy link

@coderabbitai coderabbitai bot Oct 27, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using global static variables

The use of global static variables URL and port can lead to issues in multithreaded environments or when the values need to be changed dynamically.

Consider encapsulating them within the Github class or passing them as parameters to the methods:

-namespace github {
-
-static inline std::string URL = "api.github.com";
-static inline int port = 443;
-
-}  // namespace github

And within the Github class:

template <bool https = true>
class Github {
 private:
  static constexpr const char* URL = "api.github.com";
  static constexpr int port = 443;
  // ...
};

Copy link
Owner Author

Choose a reason for hiding this comment

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

these values cannot be changed at runtime

Copy link

Choose a reason for hiding this comment

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

@PhilipDeegan, understood.


✏️ Learnings added
Learnt from: PhilipDeegan
PR: PhilipDeegan/maiken#108
File: inc/maiken/github.hpp:48-49
Timestamp: 2024-10-28T10:48:07.309Z
Learning: In the `maiken` project, when constants cannot be changed at runtime, it's acceptable to define them as `static inline` variables in a namespace.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


} // namespace github

template <bool https = true>
class Github {
private:
static bool IS_SOLID(std::string const& r) {
return r.find("://") != std::string::npos || r.find("@") != std::string::npos;
}

auto static request(std::string const& path) {
if constexpr (https)
return mkn::ram::https::Get(github::URL, path, github::port);
else
return mkn::ram::http::Get(github::URL, path, github::port);
}

public:
static bool GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo,
std::string& branch);
Expand All @@ -53,6 +73,150 @@ class Github {
std::string& branch);
static bool GET_LATEST(std::string const& repo, std::string& branch);
};

template <bool https>
bool Github<https>::GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo,
std::string& branch) {
bool b = 0;
int retry = 3;
std::stringstream ss;
ss << "repos/" << owner << "/" << repo;
while (retry-- > 0) {
request(ss.str())
.withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}})
.withResponse([&b, &branch](auto const& r) {
if (r.status() == 200) //
try {
mkn::kul::yaml::String const yaml(r.body());
KLOG(OTH) << "Github API default branch response: " << r.body();
if (yaml.root() && yaml.root()["default_branch"]) {
branch = yaml.root()["default_branch"].Scalar();
b = 1;
}
} catch (YAML::Exception const&) {
KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH invalid response received.";
}
})
.send();
if (b) return b;
KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - retrying";
using namespace std::chrono_literals;
std::this_thread::sleep_for(50ms);
}
return b;
}
Comment on lines +77 to +107
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving retry mechanism and error handling

  1. The retry count should be a named constant
  2. Error handling could be more informative
+static constexpr int MAX_RETRY_ATTEMPTS = 3;
+static constexpr auto RETRY_DELAY = std::chrono::milliseconds(50);

 bool Github<https>::GET_DEFAULT_BRANCH(...) {
   bool b = 0;
-  int retry = 3;
+  int retry = MAX_RETRY_ATTEMPTS;
   std::stringstream ss;
   ss << "repos/" << owner << "/" << repo;
   while (retry-- > 0) {
     // ... existing code ...
     if (b) return b;
-    KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - retrying";
+    KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - attempt " 
+              << (MAX_RETRY_ATTEMPTS - retry) << "/" << MAX_RETRY_ATTEMPTS;
     using namespace std::chrono_literals;
-    std::this_thread::sleep_for(50ms);
+    std::this_thread::sleep_for(RETRY_DELAY);
   }
   return b;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
template <bool https>
bool Github<https>::GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo,
std::string& branch) {
bool b = 0;
int retry = 3;
std::stringstream ss;
ss << "repos/" << owner << "/" << repo;
while (retry-- > 0) {
request(ss.str())
.withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}})
.withResponse([&b, &branch](auto const& r) {
if (r.status() == 200) //
try {
mkn::kul::yaml::String const yaml(r.body());
KLOG(OTH) << "Github API default branch response: " << r.body();
if (yaml.root() && yaml.root()["default_branch"]) {
branch = yaml.root()["default_branch"].Scalar();
b = 1;
}
} catch (YAML::Exception const&) {
KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH invalid response received.";
}
})
.send();
if (b) return b;
KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - retrying";
using namespace std::chrono_literals;
std::this_thread::sleep_for(50ms);
}
return b;
}
static constexpr int MAX_RETRY_ATTEMPTS = 3;
static constexpr auto RETRY_DELAY = std::chrono::milliseconds(50);
template <bool https>
bool Github<https>::GET_DEFAULT_BRANCH(std::string const& owner, std::string const& repo,
std::string& branch) {
bool b = 0;
int retry = MAX_RETRY_ATTEMPTS;
std::stringstream ss;
ss << "repos/" << owner << "/" << repo;
while (retry-- > 0) {
request(ss.str())
.withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}})
.withResponse([&b, &branch](auto const& r) {
if (r.status() == 200) //
try {
mkn::kul::yaml::String const yaml(r.body());
KLOG(OTH) << "Github API default branch response: " << r.body();
if (yaml.root() && yaml.root()["default_branch"]) {
branch = yaml.root()["default_branch"].Scalar();
b = 1;
}
} catch (YAML::Exception const&) {
KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH invalid response received.";
}
})
.send();
if (b) return b;
KLOG(ERR) << "maiken::Github::GET_DEFAULT_BRANCH failed - attempt "
<< (MAX_RETRY_ATTEMPTS - retry) << "/" << MAX_RETRY_ATTEMPTS;
using namespace std::chrono_literals;
std::this_thread::sleep_for(RETRY_DELAY);
}
return b;
}


template <bool https>
bool Github<https>::GET_LATEST_RELEASE(std::string const& owner, std::string const& repo,
std::string& branch) {
bool b = 0;
int retry = 3;
std::stringstream ss;
ss << "repos/" << owner << "/" << repo << "/releases/latest";

while (retry-- > 0) {
request(ss.str())
.withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}})
.withResponse([&b, &branch](auto const& r) {
if (r.status() == 200) //
try {
mkn::kul::yaml::String const yaml(r.body());
if (yaml.root()["tag_name"]) {
branch = yaml.root()["tag_name"].Scalar();
b = 1;
}
} catch (YAML::Exception const&) {
KLOG(ERR) << "maiken::Github::GET_LATEST_RELEASE invalid response received.";
}
})
.send();
if (b) return b;
KLOG(ERR) << "maiken::Github::GET_LATEST_RELEASE failed - retrying";
using namespace std::chrono_literals;
std::this_thread::sleep_for(50ms);
}
return b;
}
Comment on lines +109 to +139
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider extracting common retry logic

The retry mechanism is duplicated across multiple methods. Consider extracting it into a reusable helper.

template <typename F>
static bool withRetry(const char* operation, F&& func) {
  int retry = MAX_RETRY_ATTEMPTS;
  while (retry-- > 0) {
    if (func()) return true;
    KLOG(ERR) << "maiken::Github::" << operation << " failed - attempt "
              << (MAX_RETRY_ATTEMPTS - retry) << "/" << MAX_RETRY_ATTEMPTS;
    std::this_thread::sleep_for(RETRY_DELAY);
  }
  return false;
}

Usage example:

bool Github<https>::GET_LATEST_RELEASE(...) {
  std::stringstream ss;
  ss << "repos/" << owner << "/" << repo << "/releases/latest";
  bool success = false;
  
  return withRetry("GET_LATEST_RELEASE", [&]() {
    request(ss.str())
      .withHeaders({...})
      .withResponse([&success, &branch](auto const& r) {
        // existing response handling
        success = true;
      })
      .send();
    return success;
  });
}


template <bool https>
bool Github<https>::GET_LATEST_TAG(std::string const& owner, std::string const& repo,
std::string& branch) {
bool b = 0;
int retry = 3;
std::stringstream ss;
ss << "repos/" << owner << "/" << repo << "/git/tags";
while (retry-- > 0) {
request(ss.str())
.withHeaders({{"User-Agent", "Mozilla not a virus"}, {"Accept", "application/json"}})
.withResponse([&b, &branch](auto const& r) {
if (r.status() == 200) //
try {
mkn::kul::yaml::String const yaml(r.body());
if (yaml.root().Type() == 3) {
if (yaml.root()["ref"]) {
branch = yaml.root()["ref"].Scalar();
b = 1;
}
}
Comment on lines +155 to +160
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check for empty YAML root node type

In GET_LATEST_TAG, the code checks if yaml.root().Type() == 3, but it's not clear what Type() == 3 represents.

Use the appropriate YAML::NodeType enumeration for clarity:

if (yaml.root().Type() == YAML::NodeType::Sequence) {
  if (!yaml.root().empty() && yaml.root()[0]["ref"]) {
    branch = yaml.root()[0]["ref"].Scalar();
    b = 1;
  }
}

} catch (YAML::Exception const&) {
KLOG(ERR) << "maiken::Github::GET_LATEST_TAG invalid response received.";
}
})
.send();

if (b == 1) {
auto bits(mkn::kul::String::SPLIT(branch, "/"));
branch = bits[bits.size() - 1];
return b;
}
Comment on lines +167 to +171
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve branch name extraction robustness

The current branch name extraction assumes the last segment is the tag name, but this might not always be true.

Consider using a more robust approach:

-    if (b == 1) {
-      auto bits(mkn::kul::String::SPLIT(branch, "/"));
-      branch = bits[bits.size() - 1];
-      return b;
-    }
+    if (b == 1) {
+      if (branch.empty()) return false;
+      size_t last_slash = branch.find_last_of('/');
+      if (last_slash != std::string::npos) {
+        branch = branch.substr(last_slash + 1);
+      }
+      return !branch.empty();
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (b == 1) {
auto bits(mkn::kul::String::SPLIT(branch, "/"));
branch = bits[bits.size() - 1];
return b;
}
if (b == 1) {
if (branch.empty()) return false;
size_t last_slash = branch.find_last_of('/');
if (last_slash != std::string::npos) {
branch = branch.substr(last_slash + 1);
}
return !branch.empty();
}

KLOG(ERR) << "maiken::Github::GET_LATEST_TAG failed - retrying";
using namespace std::chrono_literals;
std::this_thread::sleep_for(50ms);
}
return b;
}

template <bool https>
bool Github<https>::GET_LATEST(std::string const& repo, std::string& branch) {
#ifndef _MKN_DISABLE_SCM_

std::vector<std::function<decltype(GET_DEFAULT_BRANCH)>> gets{
&GET_DEFAULT_BRANCH, &GET_LATEST_RELEASE, &GET_LATEST_TAG};
Comment on lines +183 to +184
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Clarify the use of function pointers in the vector

Storing member function pointers in a vector may lead to issues due to the different calling conventions.

Adjust the vector to store std::function objects bound to the class instance or use static methods properly.

std::vector<std::function<bool(std::string const&, std::string const&, std::string&)>> gets{
    &Github::GET_DEFAULT_BRANCH, &Github::GET_LATEST_RELEASE, &Github::GET_LATEST_TAG};

Ensure that the methods are correctly referenced.

std::vector<size_t> orders{0, 1, 2};
if (_MKN_GIT_WITH_RAM_DEFAULT_CO_ACTION_ == 1) orders = {1, 2, 0};

std::vector<std::string> repos;
if (IS_SOLID(repo))
repos.push_back(repo);
else
for (std::string const& s : Settings::INSTANCE().remoteRepos()) repos.push_back(s + repo);
Comment on lines +189 to +192
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Refine repository URL parsing logic

The current logic for parsing repository URLs assumes specific patterns and may fail for edge cases.

Consider using a regular expression or a dedicated URL parsing library to handle different URL formats more robustly.

For example, you can use std::regex to extract the owner and repo names.

for (std::string const& s : repos) {
if (s.find("github.com") != std::string::npos) {
std::string owner = s.substr(s.find("github.com") + 10);
if (owner[0] != '/' && owner[0] != ':') {
KERR << "Repo \"" << s << "\" is invalid - skipping";
continue;
}
owner.erase(0, 1);
if (owner.find("/") != std::string::npos) owner = owner.substr(0, owner.find("/"));

if (owner.empty()) {
KERR << "Invalid attempt to perform github lookup";
continue;
}

Comment on lines +189 to +207
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve GitHub URL parsing and error handling

The current URL parsing logic could be more robust and provide better error messages.

Consider using a more structured approach:

struct GitHubRepo {
  std::string owner;
  std::string name;
  
  static std::optional<GitHubRepo> parse(const std::string& url) {
    static const std::regex github_url_regex(
      R"(github\.com[/:]([\w.-]+)/([\w.-]+)(?:\.git)?(?:/)?$)");
    
    std::smatch matches;
    if (std::regex_search(url, matches, github_url_regex)) {
      return GitHubRepo{matches[1], matches[2]};
    }
    return std::nullopt;
  }
};

Usage:

-      std::string owner = s.substr(s.find("github.com") + 10);
-      if (owner[0] != '/' && owner[0] != ':') {
-        KERR << "Repo \"" << s << "\" is invalid - skipping";
-        continue;
-      }
-      owner.erase(0, 1);
+      auto parsed_repo = GitHubRepo::parse(s);
+      if (!parsed_repo) {
+        KERR << "Invalid GitHub URL format: " << s;
+        continue;
+      }

for (auto const& order : orders)
if (gets[order](owner, repo, branch)) return 1;
}
}
#else
KEXIT(1,
"SCM disabled, cannot resolve dependency, check local paths and "
"configurations");
#endif
return 0;
}

} // namespace maiken

#endif //_MKN_WITH_MKN_RAM_
Expand Down
9 changes: 8 additions & 1 deletion mkn.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
name: mkn
version: master
property:
DATE: 07-OCT-2024
DATE: 27-OCT-2024
maiken_location: ${MKN_HOME}/app/mkn/${version}
maiken_scm: https://github.com/mkn/mkn
self.deps: mkn.kul
Expand Down Expand Up @@ -85,3 +85,10 @@ profile:

- name: lib_test
dep: mkn&${maiken_location}(${maiken_scm})[mod]


- name: github
parent: headers
dep: parse.yaml
main: test/github/test.cpp
with: mkn.ram
142 changes: 0 additions & 142 deletions src/maiken/scm/github.cpp

This file was deleted.

Binary file added test/github/__pycache__/mock_api.cpython-312.pyc
Binary file not shown.
Loading
Loading