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

feature/workday_scraper #67

Merged

Conversation

noameron
Copy link
Contributor

Added support for generic Workday scraper, and updated requirements.

I am facing issues trying to build docker, due to chrome / chromeDriver.

@nicobrenner
Copy link
Owner

Added support for generic Workday scraper, and updated requirements.

I am facing issues trying to build docker, due to chrome / chromeDriver.

Saw your comment on the #23 ticket. Is that an error inside docker? Is your docker instance not able to install chrome? Are you able to run it using python + venv?

@nicobrenner
Copy link
Owner

Just looked over the PR, I like it. Please include a test for the scraper. It can be something very simple and straightforward that tests if the scraper is properly retrieving info from the site. Thank you

@noameron
Copy link
Contributor Author

@nicobrenner Hey! I am able to run it locally through venv, and I also managed to build with docker just now but running it is causing some issues atm.

I pushed a commit with a successful docker build

Copy link
Owner

@nicobrenner nicobrenner left a comment

Choose a reason for hiding this comment

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

Thank you @noameron for this PR. Excellent work. One last request: please update the README.md file to include an update at the top. It should be 1 or 2 sentences about what is new. Extra brownie points if you also make a short video using it 🙏🏽🤗:

image

@@ -10,6 +10,11 @@ WORKDIR /commandjobs
# Install any needed packages specified in requirements.txt
RUN pip3 install --no-cache-dir -r config/requirements.txt

# Install required packages, including Chromium and ChromeDriver
Copy link
Owner

Choose a reason for hiding this comment

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

Excellent :)

@@ -14,7 +16,7 @@
class WorkdayScraper:
def __init__(self, db_path='job_listings.db', update_func=None, done_event=None, result_queue=None):
self.db_path = db_path
self.driver = webdriver.Chrome(options=self.get_selenium_configs())
self.driver = webdriver.Chrome(service=Service(ChromeDriverManager().install()), options=self.get_selenium_configs())
Copy link
Owner

Choose a reason for hiding this comment

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

Very helpful 👍🏼

"🕸 Scrape \"Work at a Startup jobs\"",
"🕸 Scrape \"Workday\"",
Copy link
Owner

Choose a reason for hiding this comment

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

😄

@noameron noameron requested a review from nicobrenner November 17, 2024 18:31
@noameron
Copy link
Contributor Author

@nicobrenner Hey! I just pushed a unit test for verifying the base elements of all job listings, I think next PR I should add a workflow for triggering the unit tests on each PR push.

Other than that everything seems to work on my end when i checked

@noameron
Copy link
Contributor Author

Where should I update on the README regarding the new scarper for workday?

@nicobrenner
Copy link
Owner

Where should I update on the README regarding the new scarper for workday?

Please add an entry at the top of the Updates > Building in public: section. It should be 2-3 sentences about what the change does in very simple terms so that most people reading the README can easily understand it

Optional: it would be great to have a short screencast of you demoing the new changes within commandjobs, you could add it with the above entry

Thank you

@nicobrenner
Copy link
Owner

@nicobrenner Hey! I just pushed a unit test for verifying the base elements of all job listings, I think next PR I should add a workflow for triggering the unit tests on each PR push.

Other than that everything seems to work on my end when i checked

Excellent. It's looking good. With the update to the README, the PR should be ready to merge 👍🏼

@noameron
Copy link
Contributor Author

Just pushed the readme update! @nicobrenner

Copy link
Owner

@nicobrenner nicobrenner left a comment

Choose a reason for hiding this comment

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

Great! Approved 👍🏼✅

@nicobrenner nicobrenner merged commit 3f54f05 into nicobrenner:master Nov 18, 2024
@noameron noameron deleted the feature/nvidia_workday_scraper branch November 18, 2024 10:46
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