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

Kafka initial implementation #1416

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Kafka initial implementation #1416

wants to merge 2 commits into from

Conversation

szachovy
Copy link
Member

@szachovy szachovy commented Oct 7, 2024

@szachovy szachovy self-assigned this Oct 7, 2024
Copy link
Contributor

@Bischoff Bischoff left a comment

Choose a reason for hiding this comment

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

see remarks

. \
"/home/appuser/venv/bin/activate" \
&& \
pip \
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if we could use a SUSE package rather than using pip

-m \
venv \
"/home/appuser/venv" \
&& \
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use several commands within RUN section?

if you want to bail out at first error, there are several solutions:

  • set -e at the beginning
  • rc = <command1>
    [ rc == 0 ] && rc = <command2>
    ...
  • or maybe that's just the default behaviour in a dockerfile ?

Copy link
Contributor

@NamelessOne91 NamelessOne91 Oct 7, 2024

Choose a reason for hiding this comment

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

I think we can split this into multiple RUN commands, if we want to.
It should also improve layers caching for Docker at the cost of a final image which is (probably) slightly bigger

 
RUN python3 -m venv /home/appuser/venv
RUN . /home/appuser/venv/bin/activate \
    && pip install confluent_kafka==2.3.0 GitPython==3.1.43 requests==2.32.3
RUN git clone https://github.com/SUSE/susemanager-ci

@@ -0,0 +1,62 @@
# Kafka automation concept

Messaging system to speed up business processes via services API.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does that mean ?

@@ -0,0 +1,25 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

extra blank line

logging.error(f'Request is too big, perhaps too many RRs to be accepted generated big JSON, run pipeline manually using generated custom_repositories.json in {os.getcwd()}')
return None

def pipeline_status(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicky, but maybe is worth changing this method name and signature to something like :

def pipeline_enabled(self) -> bool:

str(incident['incident']['incident_id'])
for incident in incidents['data']
)
subprocess.run(
Copy link
Contributor

@NamelessOne91 NamelessOne91 Oct 7, 2024

Choose a reason for hiding this comment

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

Having set check=True, if the JSON generating scripts fails, for whatever reason, this function call will raise a subprocess.CalledProcessError

Maybe we should handle the possibility of a failure here, or where the generate_custom_repositories function is called, and at least log some useful info.

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.

3 participants