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 Boat Simulator and Enhance Build Tasks #114

Merged
merged 23 commits into from
Jun 11, 2023

Conversation

DFriend01
Copy link
Contributor

Description

Related to this issue. Adds the boat simulator and modifies the build script.

The following was modified:

  • Added the boat simulator to the project repositories
  • Modified the build script to allow for building specific packages rather than all of them at once
  • Modified the VS Code tasks:
    • Build All is the old Build task
    • Build task prompts for specific packages to be built

Verification

  • Sailboat Simulator:
    • Rebuilt the container to see if the boat simulator repo is cloned
  • Build Script:
    • The build script ran without any arguments operates as it originally did -- it builds all packages
    • CTRL + SHIFT + B brings up both Build All and Build tasks as build options

@DFriend01 DFriend01 added enhancement New feature or request ctrl Controller team labels Apr 30, 2023
@DFriend01 DFriend01 requested a review from patrick-5546 as a code owner April 30, 2023 06:32
@DFriend01 DFriend01 self-assigned this Apr 30, 2023
@patrick-5546 patrick-5546 mentioned this pull request May 24, 2023
21 tasks
Copy link
Member

@patrick-5546 patrick-5546 left a comment

Choose a reason for hiding this comment

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

Tested out the new build task: it works perfectly for one package, but not multiple packages. I think this is because packages are separated by spaces, which in bash delimits arguments. My suggestion would just be to keep it simple: delete the packages task input and use the package. I don't think it's necessary to be able to specify multiple packages to build, would be easier just build everything using the regular build task.

Also flake8 started failing, I'm not sure what changed.

@DFriend01
Copy link
Contributor Author

DFriend01 commented May 25, 2023

Wait to merge until this issue is resolved:
UBCSailbot/boat_simulator#6

It should resolve the linting errors in this PR

Copy link
Member

@patrick-5546 patrick-5546 left a comment

Choose a reason for hiding this comment

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

LGTM, will approve once comments resolved

@patrick-5546 patrick-5546 removed the ctrl Controller team label Jun 9, 2023
Copy link
Member

@patrick-5546 patrick-5546 left a comment

Choose a reason for hiding this comment

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

CI will pass once you merge in your boat_simulator PR and rerun failed jobs

Copy link
Member

@patrick-5546 patrick-5546 left a comment

Choose a reason for hiding this comment

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

One more idea that I had regarding the build tasks: incorporating quick_build.sh. There are multiple ways that this can be done, I'll let you decide what to do.

What do you think about combining quick_build.sh and build.sh by adding an argument to build.sh? Also if we keep quick_build.sh I think we should rename it build_quick.sh so that it is next to build.sh alphabetically. The deployment README/script would have to be updated accordingly.

If you don't have time to do this right now can you write an issue for this?

@patrick-5546 patrick-5546 mentioned this pull request Jun 11, 2023
2 tasks
@patrick-5546 patrick-5546 enabled auto-merge (squash) June 11, 2023 01:33
@patrick-5546 patrick-5546 merged commit de36c3c into main Jun 11, 2023
@patrick-5546 patrick-5546 deleted the user/dfriend01/add_boat_simulator branch June 11, 2023 22:18
@patrick-5546 patrick-5546 mentioned this pull request Jun 14, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request infrastructure
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants